interpreter: Use symboltable lookup in the interpreter

[jumerckx]

This is to support nested SymbolRefAttrs.

[jumerckx]

With this change, xdsl-run doesn't find the main entry point when it is nested deeper than the top-level module. For example, here:

builtin.module {
  riscv.assembly_section ".data" {
  ...
  }
  riscv.assembly_section ".text" {
    riscv.directive ".globl" "main"
    riscv.directive ".p2align" "2"
    riscv_func.func @main() {
    ...
    }
  }
}

I think this currently only happens with the riscv dialect.
Potential solution is to have a special find_main_entrypoint that goes over all symbols and finds the unique main symbol?
(or should users be forced to specify the nested symbol for these cases?)

[compor]

This is not exactly the same lookup, as the interpreter maintains a lookup via the symbol_table property.
When using the SymbolTable.lookup_symbol you indicate the operation from where to start the search and move "outwards"; here this seems to go straight to the containing module.

Not sure why something like this wouldn't do the trick? (haven't tested it):

if isinstance(symbol, SymbolRefAttr):
    symbol = symbol.string_value()
if symbol in self.symbol_table:
    return self.symbol_table[symbol]
...

[jumerckx]

I see what you're saying. So this lookup should start looking at the level of the call site.

Simply converting to string does not suffice. The symbol_table does not contain the full nested symbol refs, but rather, only the tail of a symbol path. A call to a nested symbol "@parent::@child" will not be found in the map because it only contains "@child" currently.
If I understand correctly, in general you can't have a simple mapping from unique string to operation because, depending on scope, a symbol has a different name.

All this is under the assumption that symbols can shadow each other, i.e.

func @symbol

module @parent {
	func @symbol
}

Not entirely sure that is true though.

[compor]

Hmm, I see; that lookup as you described happens via the SymbolTable::lookup; to me then sounds like the interpreter lookup needs a fix

[jumerckx]

Is there a way to get the current operation being handled by the interpreter, some sort of program counter. If not I can add it and use that value instead of self.module to seed the lookup?

[superlopuh]

This definitely seems like an upgrade in functionality and we need to figure out how to fix riscv to make it work properly with these symbols. The existing symbol table helper should be deleted IMO and just replaced with this, the clients might as well call it directly, and give the relevant root op. I can see how to refactor RISC-V here to work better, and that seems like a pre-requisite to merging this.

[compor]

I agree, I'd rather use the same lookup structure, and we have that via the SymbolTable.

[superlopuh]

I've fixed the remaining failing tests, I forgot to update during my symbol table implementation for riscv_func, now everything seems to work!

[superlopuh]

just a dedent

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.98%. Comparing base (39259a5) to head (2920b5e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
xdsl/interpreter.py	72.72%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4101      +/-   ##
==========================================
- Coverage   88.98%   88.98%   -0.01%     
==========================================
  Files         326      326              
  Lines       44585    44576       -9     
  Branches     5493     5489       -4     
==========================================
- Hits        39676    39666      -10     
  Misses       3553     3553              
- Partials     1356     1357       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[superlopuh]

@jumerckx could you please add two minimal tests for the lines missing code coverage before you merge?

[superlopuh]

Oh actually I just realized that we should add a test for the reason we're adding this functionality, which is a nested symbol. Should be GTG after that.

[jumerckx]

I added a test but it also depends on func dialect.
In the process I also got rid of the conversion from callee symbol to string in func.CallOp's run method.