dialects: (hw) Add InnerSymbolTable and InnerRefNamespace to HW dialect

[lucjaulmes]

This is another standalone part of #1704 (which you can refer to for broader context). It introduces 2 main classes, InnerSymbolTable and InnerRefNamespace (which are too interconnected to add separately), as well as 2 smaller (and similarly intricated) InnerSymbolTableCollection and InnerRefUserOpInterface.

Note that I’ve tried reducing as much as I could but this is as small as I can make this contribution (~150 loc / 4 classes + tests).

The PR introduces a verification step, called verifyInnerRefs:

• I think the most explicit description is from the Symbol and Inner Symbol Rationale document:

  InnerRefNamespace

  [As class:] Combines InnerSymbolTableCollection with a SymbolTable for resolution of InnerRefAttrs, primarily used during verification as argument to the verifyInnerRefs hook which operations may use for more efficient checking of InnerRefAttrs.

  [As trait:] InnerRefNamespace is used by Operations to define a new scope for InnerRef’s. Operations with this trait must also be a SymbolTable. Presently the only user is CircuitOp.

  Inner symbols are more costly than normal symbols, precisely from the relaxation of MLIR symbol constraints. Since nested regions are allowed, finding all operations defining an inner_sym requires a recursive IR scan.
  Verification is likewise trickier, partly due the significant increase in non-local references.

  For this reason, verification is driven as a trait verifier on InnerRefNamespace which constructs and verifies InnerSymbolTables in parallel, and uses these to conduct a per-InnerSymbolTable parallelized walk to verify references by calling the verifyInnerRefs hook on InnerRefUserOpInterface operations.

  I’ve tried to summarise this in the InnerRefNamespace docstring.

• I still have a couple of doubts on how/where this verify_inner_refs() should be called. It seems to be called primarily from verifyRegionTrait, though I couldn’t find what that corresponds to in xdsl terminology. Just a verify() or verify_()?

• verify_inner_refs is additionally called in a VerifyInnerRefNamespacePass HW transform, but I left that out for now as it seems we can do without it (at least for now).

  • The doc for VerifyInnerRefNamespacePass says it’s used to “to drive verification of operations that want to be InnerRefNamespace's but don't have the trait” using InnerRefNamespaceLike to find applicable operations.
  • InnerRefNamespaceLike interface in turn says “Classify operations that are InnerRefNamespace-like, until structure is in place to do this via Traits. […] Prefer putting the trait on operations here or downstream.”.

@PapyChacal since AFAIU you worked on the main symbol table, can you try and see if I tested this right, and find what more could be tested in here? Especially I think my attempts at creating synthetic ops using the interfaces in the python tests might be clunky/simplifiable.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (176b7de) 89.24% compared to head (919422e) 89.45%.
Report is 275 commits behind head on main.

Files	Patch %	Lines
xdsl/dialects/hw.py	98.27%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1780      +/-   ##
==========================================
+ Coverage   89.24%   89.45%   +0.20%     
==========================================
  Files         264      317      +53     
  Lines       32585    38702    +6117     
  Branches     4826     5730     +904     
==========================================
+ Hits        29081    34619    +5538     
- Misses       2798     3280     +482     
- Partials      706      803      +97     

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

[superlopuh]

not familiar with the dialect, but code looks reasonable

[PapyChacal]

Left some testing comments as asked for! Looks good overall

You have some untested lines, codecov report them nicely, you can click on the filenames on its PR comment to see which ones are not tested.

Note that I'm giving both those comments as you requested specific feedback; In my opinion, as my approval says, it does look way good enough to move on for me.
This is in a pretty clean state already, I would say you can merge as is and you can start using the dialect and fix anything that would show up on the go.

Now, I won't block from trying to merge it directly in a perfect state either 😉

[lucjaulmes]

Any takes on whether verify_region_trait() should stay as is or be changed to just verify()? @compor maybe?

[compor]

Any takes on whether verify_region_trait() should stay as is or be changed to just verify()? @compor maybe?

Good point, I missed this. I'd say to rename to verify since this is both a class and a trait in CIRCT.

[lucjaulmes]

It’s mostly done now, just a couple of tests to add on the lookup functions.

Unfortunately there are some exceptions I can’t manage to trigger as they are redundant with other checks elsewhere. Should I keep these checks or remove them?

One example is checking InnerRefNamespace operations only have a single region/block, which is redundant with SymbolTable checks (and InnerRefNamespace enforces the operation is also a SymbolTable).

[lucjaulmes]

• Found a solution for my problem (writing it out seemed to help?), which is to use what upstream does: InnerSymbolTableCollection maps from operations to InnerSymbolTable instances, instead of just being a set of operations and using trait lookup.
• This has somewhat complicated implications for the so-far untested bits (lookup functions) so I removed those and will add them in an upcoming PR.
• Rebased on master

[lucjaulmes]

This is ready for merging - remaining non-covered lines are either just ... statements or checks that can only be true because they’re already enforced somewhere else as discussed previously. I’ve left them both for their documentation value and because if the tests that currently enforce them change (say we’re using a subclass of SymbolTable with some relaxations?) then it’s still clear they’re needed here.

[tobiasgrosser]

@luisacicolini, could you do a first pass on this PR.

[PapyChacal]

Alternately, I could just define 2 different classes, one for the trait and one for the lookups. That would be simpler but break the larger pattern. @PapyChacal advocates for this and I think it is sensible. But are there reasons to not do it?

This is not what I had in mind, but I don't see any issue with this if it helps 🙂 Honestly, do however you feel is most manageable for now. Our Traits/Interfaces implementation is far from optimal, and after having close looks with you on this, I believe your work is suffering from its flaws more than any so far. (as-in, what you're trying to do, not the way you do it; I do not see obvious answers with this state of things TBH).

To repeat it on the record, One thought I have is that you might want to avoid having actual state in your table, and just do naive lookup on the IR. Building the state is okay, but updating it with IR changes is another problem, for which xDSL lacks infrastructure so far. And to repeat my point too, I believe this can be okay for a while, as we usually don't work with heavy loads in xDSL. This will depends on the planned usage of this dialect I guess 🙂

[lucjaulmes]

The lookups are less long-lived than what we initially discussed, so I think updating isn’t too much of an issue here.

@math-fehr do you have insight in design decisions to weigh in on this? If not, can you suggest someone else?

To summarize the issue: the interfaces I’m trying to copy from circt hold state (lookup tables) which doesn’t seem possible with current xDSL implementation (due to uniqueness and frozen dataclass semantics). This implementation uses a field(compare=False, …) which isn’t used when the class is instantiated as a trait, and is used when the class is instantiated for lookups.

It’s what fits with the code restrictions but I’m a little uneasy introducing this double-use of a single class without anyone properly saying it’s the best way forward.

[math-fehr]

@lucjaulmes Can you show me where an interface is keeping state? This is something that is not allowed in MLIR AFAIK.
In C++, the interfaces are defined on the Operation classes, not on the operations themselves, so I'm not even sure how would that be possible.

[lucjaulmes]

The SymbolTable and InnerSymbolTableCollection contain the lookup info themselves @math-fehr , e.g. for the former:

private:
  using TableTy = DenseMap<StringAttr, InnerSymTarget>;
  /// Construct an inner symbol table for the given operation,
  /// with pre-populated table contents.
  explicit InnerSymbolTable(Operation *op, TableTy &&table)
      : innerSymTblOp(op), symbolTable(table){};


  /// This is the operation this table is constructed for, which must have the
  /// InnerSymbolTable trait.
  Operation *innerSymTblOp;


  /// This maps inner symbol names to their targets.
  TableTy symbolTable;

And for the latter:

private:
  /// This maps Operations to their InnnerSymbolTable's.
  DenseMap<Operation *, std::unique_ptr<InnerSymbolTable>> symbolTables;

Some more relevant code is linked / quoted in this comment aboven

[math-fehr]

The SymbolTable and InnerSymbolTableCollection contain the lookup info themselves @math-fehr , e.g. for the former:

private:
  using TableTy = DenseMap<StringAttr, InnerSymTarget>;
  /// Construct an inner symbol table for the given operation,
  /// with pre-populated table contents.
  explicit InnerSymbolTable(Operation *op, TableTy &&table)
      : innerSymTblOp(op), symbolTable(table){};


  /// This is the operation this table is constructed for, which must have the
  /// InnerSymbolTable trait.
  Operation *innerSymTblOp;


  /// This maps inner symbol names to their targets.
  TableTy symbolTable;

And for the latter:

private:
  /// This maps Operations to their InnnerSymbolTable's.
  DenseMap<Operation *, std::unique_ptr<InnerSymbolTable>> symbolTables;

Some more relevant code is linked / quoted in this comment aboven

I see, but they are not used as an interface right? My understanding is that they are returned by the interface right?
SO it would be having an interface that returns this structure

[math-fehr]

I have a meeting now, but I'll try to dig a bit more to understand what's going on!

[math-fehr]

For instance, to me, these structures are populated here:
https://github.com/llvm/circt/blob/27f35ba7c569dea50467c24427cfb430d3bb45bf/lib/Dialect/HW/InnerSymbolTable.cpp#L156

They are not stored anywhere in the interface AFAIK?

[lucjaulmes]

It may be part of the trait more than the interface in itself, but AFAICT those 2 concepts are merged in xDSL. Or I may be confused and missing something?

[math-fehr]

So the difference between a trait and an interface is that interfaces have "virtual" methods, while trait do not.
In xDSL I merged both because that just mean that traits are interfaces that have no "virtual" methods.

So to me, I think this is a different problem?

[lucjaulmes]

My problem is I can’t hold state in xDSL traits because they’re frozen singletons @math-fehr, and they seem to be the logical place for this to go due to how MLIR concepts are mapped to xDSL (in my understanding at least?)

Are you saying there’s some place I missed where I this lookup info could logically go? Should I use the same class for both usages (current proposal)? Or should I split MLIR (rather, CIRCT) traits into xDSL traits and other “friend” classes? Or something else entirely?

[math-fehr]

My problem is I can’t hold state in xDSL traits because they’re frozen singletons @math-fehr, and they seem to be the logical place for this to go due to how MLIR concepts are mapped to xDSL (in my understanding at least?)

Are you saying there’s some place I missed where I this lookup info could logically go? Should I use the same class for both usages (current proposal)? Or should I split MLIR (rather, CIRCT) traits into xDSL traits and other “friend” classes? Or something else entirely?

So my understanding is that MLIR has the exact same concept, traits are frozen (and so are interfaces).

My understanding is that the classes you sent me are like "friend" classes that are used by the actual interface?
Or that this is not an interface at all, but something else.

[math-fehr]

Sorry about that, missclick.

I'll try to read that MLIR code to really understand what's going on!

[math-fehr]

Okay, I think I got the confusion now!

There are actually two InnerSymbolTrait classes. One is in the OpTrait namespace, and one is not.
The one you sent is the non-trait one, and is the one with a state.
The trait one is here. The trait has no state here.

If you look at the non-trait constructor, you can see that the InnerSymbolTable class (the one with the state) is created at an operation location.

So you should have two structures. One xDSL trait that has no state, and which can be used to create the "state" class

[tobiasgrosser]

@lucjaulmes, any update on this?

[lucjaulmes]

Rebased on master and squashed in a8ddbbd, should be good to merge.

(Incremental changes with last version of PR can be viewed in e3eec8b.)

[superlopuh]

This looks great! I'm not sure how exhaustively we want to check the functionality added here, I would say that it's pretty well tested already, I just thought I'd suggest looking at the codecov to be sure that the verification errors that aren't raised in the tests aren't as critical as the ones covered in the test already.