Symbols returning results, upcoming verification failures

[jpienaar]

llvm/llvm-project#168390 makes these a verification failure. Currently all CalyxCell instances (which are all CalyxPrimitive ones) are marked as symbols.

[jpienaar]

Same is true for SSP Operation

[uenoku]

I think Calyx's use cases could be replaced with inner symbols. Not sure about SSP. It looks like SSA producing symbol is by-design and may not compose well with InnerSymbol (https://github.com/llvm/circt/blob/main/docs/Dialects/SSP/RationaleSSP.md#use-of-ssa-operands-and-symbol-references-to-encode-dependences).

CC: @jopperm @rachitnigam @cgyurgyik @andrewb1999 @jiahanxie353

[jpienaar]

I don't know SSP and don't know the distinction between auxiliary or not here - I'd have though one could do a variadic operand to all these ops which represent the auxiliary and then be purely SSA based. Alternative could be to just use StringAttr and then manage one's own symbol table like thing (that does require a bit of duplication though).

[jopperm]

I'll look into alternatives for SSP. When is the next LLVM bump scheduled? The new, stricter checking is at runtime, correct? Could we temporarily disable the SSP tests? (SSP's main value is for testing the scheduling infra; I'm currently not aware of any other users.)

[jopperm]

I don't know SSP and don't know the distinction between auxiliary or not here - I'd have though one could do a variadic operand to all these ops which represent the auxiliary and then be purely SSA based. Alternative could be to just use StringAttr and then manage one's own symbol table like thing (that does require a bit of duplication though).

Thanks, I'll explore these ideas. Having extra SSA operands seems neat, but that conflicts with an early design choice in the scheduling infra: the dependence graph is an overlay on the SSA graph, but sometimes a scheduling problem needs to model additional ("auxiliary") dependences (e.g. between memory accesses). These deps are managed separately from the implicit SSA dependences, and I chose symbol references to encode them in SSP. On the hand, SSP's scope is quite limited, so I think StringAttr's and some additional logic in the import/export code for scheduling problem could work.

[jpienaar]

Ahhh that's what auxiliary means here. Gotcha. OK, that makes sense.

Temporarily disabling would work yes. It is done on verification, but if the tests are disabled then no issues.

[jpienaar]

I think easiest is to do the string trick (I mean it is still symbolattr, but its just not symbol ops).

The other option that comes to mind now is little wrapper ops, so doing

%1 = ssp.dep(aux=%x) {
  %0 = regular op
  ssp.dep_yield %0
}

(and one could pretty print this to be %1 = ss.dep regular_op %x (%1)). But its then just overlay of values over the original graph and easy to undo. These ops do interfere with regular pattern matches unless these are thought to look through.

[uenoku]

Interestingly, simply removing the Symbol interface passes all tests (a7853a3) because MLIR's SymbolTable implementation generally uses attribute lookups (https://github.com/llvm/llvm-project/blob/c2242bdea9cc4593ad46689e1dcc05645045f908/mlir/lib/IR/SymbolTable.cpp#L495-L496) rather than casting to SymbolOpInterface. This means operations can be registered in the symbol table without implementing SymbolOpInterface, as long as they have the sym_name attribute 😅 Though I don't think we want to use this property as workaround.

[jpienaar]

Haha, yes, but may enable a short term fix until addressed properly.