Block folders and canonicalizers when operations have metadata

Hi all,

In CIRCT, there is a situation[0, 1] where we want to disable optimizations for operations with metadata. More specifically, System Verilog has a mechanism called “SV attributes’’ (≠ SV dialect attributes) to attach metadata to almost arbitrary expressions, such as c ? (* foo *) a : b. We are considering using optional attributes to represent these attributes, e.g. comb.mux %c, %a, %b {sv.attributes=”foo”}. Since SV attributes are in general important to users, we want to block optimization if the op has “sv.attributes” attribute. The naive implementation to block optimizations would be to add guards to every folder and canonicalizer, e.g:

LogicalResult AddOp::canonicalize(AddOp op, …) {
  // Block when the op has SV attributes.
  if (hasSVAttributes(op))
    return failure();
  …

However, with this implementation it is necessary to add this boilerplate everywhere, which looks very janky so I’m wondering if there is a cleaner way we can do it.
Or if there is no such mechanism, do you think it is a good idea to introduce some OpInterface in MLIR to guard folders and canonicalizers so that we can define from ODS?

Preservation of SV attributes here is very similar to the preservation of llvm metadata, so I’m not 100% sure that optional attributes are the best way to represent this kind of metadata either.

Any suggestion or comment is highly appreciated. Thanks!

[0] [SV/Python/ExportVerilog] Allow SVAttributes to be attached everywhere by uenoku · Pull Request #3472 · llvm/circt · GitHub
[1] [SV] Generic SVAttributes · Issue #3430 · llvm/circt · GitHub

How about adding a SVAttributeOp that has a single block region and wraps the operation? This wrapping would effectively block all pattern matching that works today, the attribute is on the SVAttributeOp and it doesn’t have canonicalization that would drop it. Now it’ll get in the way of many passes, in some it’ll be working as intended and in others it will be an annoyance, and you will need passes that understands these are merely passthrough ops. The advantage here is that you don’t need to change every op’s canonicalize/fold, or every op in peephole patterns, and the parts without the attribute can be freely optimized. Previously these ops have gotten in the way of optimizations (and we had discussed making a trait to indicate such passthrough ops) but in this case that’s a feature.

Alternatively, define a new driver for patterns. Don’t use the greedy one. That still leaves the canonicalize pass though, which one could then (not saying we should) add a filter functor to. I think the other option leads to less duplication and should not to be too disruptive (you’ll have to be mindful where you drop those metadata ops but that’s true for any alternative too, and you may still want to consider not using greedy pattern drivers around when dropping).

3 Likes

How about adding a SVAttributeOp that has a single block region and wraps the operation? This wrapping would effectively block all pattern matching that works today, the attribute is on the SVAttributeOp and it doesn’t have canonicalization that would drop it.

Ah, that’s super clever! It looks like this, right?

%1 = sv.attributes.op “foo” (%c, %a, %b) ({
  ^bb(%d, %e, %f):
    %0 = comb.mux %d, %e, %f
    return %0  
});

I believe this approach should solve the problem very elegantly. I’ll try it. Thanks a lot!

Alternatively, define a new driver for patterns. Don’t use the greedy one. That still leaves the canonicalize pass though, which one could then (not saying we should) add a filter functor to.

This also makes sense to me, I’ll try this if the SVAttributeOp approach doesn’t work.

1 Like

Yes, well one can hide some more details so that the pretty form looks something like

1 = sv.attributes.op “foo” on comb.mux %c, %a, %b

It would just be sugar (and up to you if worth it, if you will allow many ops in the region , you’d have two forms etc) you’d do exactly as in your example shown but just reuse SSA IDs, so the %c you see here is really a bbarg value and not the %c feeding into the attributes op, the pretty form just hides that. This would make it more concise but at cost of some concistency & folks needing to understand this. I don’t think this would be too confusing though and makes it easier to read for me.

1 Like

It would just be sugar (and up to you if worth it, if you will allow many ops in the region , you’d have two forms etc) you’d do exactly as in your example shown but just reuse SSA IDs, so the %c you see here is really a bbarg value and not the %c feeding into the attributes op, the pretty form just hides that.

That syntax looks very reasonable, thanks!

I really like sv.attributes.op idea but on the second thought, we decided to start from defining SV attributes to be discardable because sv.attributes.op will require fair amount of architectural changes in verilog emitter. So we don’t introduce sv.attributes.op this time. However if we had a situation to block every optimizatin in the future, we will definitely revisit sv.attributes.op idea. Thank you really for the great suggestion!