[RFC] [Comb][Canonicalize] Best effort dialect attribute propagation

[7FM]

Addresses #6767.
This is a first draft how a less conservative dialect attribute propagation could be implemented.

I checked the uses of replaceOpWithNewOpAndCopyName and found one candidate where I would personally not propagate the dialect attributes:

circt/lib/Dialect/Comb/CombFolds.cpp

Lines 1685 to 1689 in c403838

	auto shlOp =
	rewriter.create<comb::ShlOp>(op.getLoc(), inputs[0], shift, false);
	replaceOpWithNewOpAndCopyName<MulOp>(rewriter, op, op.getType(),
	ArrayRef<Value>(shlOp), false);

It should be possible to opt-out of the implicit attribute propagation by explicitly using Operation* as second template parameter:

replaceOpWithNewOpAndCopyName<MulOp,  Operation*>(rewriter, op, op.getType(),
                                                  ArrayRef<Value>(shlOp), false);

Alternatively, the implementation may be changed to add a boolean template parameter to explicitly opt-in/out of the attribute propagation.

[dtzSiFive]

How do we know the dialect attributes' intended semantics are valid across the transformation?

Looks like in this PR the proposed answer is "if it's the same operation name, it's probably fine"?

I appreciate that dropping them may be problematic but I also really don't think we should be preserving attributes we don't know are safe to be preserved. While we could spin up some sort of, e.g., dialect interface to answer "should this be preserved" offhand I'm not sure that's reasonable to do generically -- "does my transformation preserve your attribute meaning?" and as such only makes sense for the subset of dialect attributes that question can be simply answered for "yes if operations are the same".

If you're folding two operations, do you merge the attributes? Which do you take? So on.

Looking through the linked discussion, I disagree that we're obligated to use heuristics to propagate these -- there's a reason the MLIR core infrastructure doesn't do this.

Is there a solution that helps your use case specifically without encoding this sort of assumption about the nature/purpose/meaning of dialect attributes generally?

[7FM]

How do we know the dialect attributes' intended semantics are valid across the transformation?

Unfortunately we don't unless they were defined within CIRCT.

Looks like in this PR the proposed answer is "if it's the same operation name, it's probably fine"?

That is the current naive implementation, yes. Of cause this is highly transformation and attribute specific, but if we want to follow a best effort approach to propagate attributes then we always need to make assumptions at some point to define our "best effort".

I appreciate that dropping them may be problematic but I also really don't think we should be preserving attributes we don't know are safe to be preserved. While we could spin up some sort of, e.g., dialect interface to answer "should this be preserved" offhand I'm not sure that's reasonable to do generically -- "does my transformation preserve your attribute meaning?" and as such only makes sense for the subset of dialect attributes that question can be simply answered for "yes if operations are the same".

If you're folding two operations, do you merge the attributes? Which do you take? So on.

Looking through the linked discussion, I disagree that we're obligated to use heuristics to propagate these -- there's a reason the MLIR core infrastructure doesn't do this.

Do you have a particular example where propagation of custom (non-CIRCT) dialect attributes is possible without certain assumptions/heuristics?
Or do you discourage custom dialect attribute propagation in general?
Maybe I misunderstood something here?

Is there a solution that helps your use case specifically without encoding this sort of assumption about the nature/purpose/meaning of dialect attributes generally?

The only thing that currently comes to my mind would be some sort of whitelisting approach for specific attribute names. However, this would create new questions, for example:

• does whitelisting affect all propagations or is a configurable granularity desired (the later would increase the burden of maintenance)

[uenoku]

I agree with Will's points. This has been discussed on MLIR forums several times and I think basically we cannot safely propagate unknown attributes. https://discourse.llvm.org/t/canonicalization-passes-dont-keep-attributes/59750/2?u=uenoku

One thing you could try is to attach an operation to retain your custom attributes before canonicalizations and pull back after canonicalizations, e.g:

  hw.module @attrsGone(in %arg0: i64, out res: i32) {
    %0 = comb.extract %arg0 from 0 : (i64) -> i42
    %1 = comb.extract %0 from 0 : (i42) -> i32
    some_dialect.keep_attribute %1 {custom_dialect.attr = "test"}
    hw.output %1 : i32
  }
=> 
  hw.module @attrsGone(in %arg0: i64, out res: i32) {
    %0 = comb.extract %arg0 from 0 : (i64) -> i32
    some_dialect.keep_attribute %0 {custom_dialect.attr = "test"}
    hw.output %0 : i32
  }

This approach is quite similar to what Debug dialect is preserving original signal names.

Or another approach might be to introduce a dialect interface to create an allow-list for attributes that can be propagated, and users can dynamically configure them while the dialect registration.

[7FM]

One thing you could try is to attach an operation to retain your custom attributes before canonicalizations and pull back after canonicalizations, e.g:

  hw.module @attrsGone(in %arg0: i64, out res: i32) {
    %0 = comb.extract %arg0 from 0 : (i64) -> i42
    %1 = comb.extract %0 from 0 : (i42) -> i32
    some_dialect.keep_attribute %1 {custom_dialect.attr = "test"}
    hw.output %1 : i32
  }
=> 
  hw.module @attrsGone(in %arg0: i64, out res: i32) {
    %0 = comb.extract %arg0 from 0 : (i64) -> i32
    some_dialect.keep_attribute %0 {custom_dialect.attr = "test"}
    hw.output %0 : i32
  }

Yes, I have thought about that solution as well and was wondering to which degree it would inhibit optimizations. Maybe this can be mitigated by adding canonicalizations to some_dialect.keep_attributes to remove operands when they are only used by some_dialect.keep_attributes.
That being said, it kind of feels wrong to define an operation that essentially acts like an attribute, but I haven't found a better solution either.

This approach is quite similar to what Debug dialect is preserving original signal names.

Thanks for the hint. I will take a look at it, specifically regarding optimizations.

[uenoku]

Thanks for the hint. I will take a look at it, specifically regarding optimizations.

Attaching operations would affect DCE, so you may need to run another DCE(probably you can use -remove-dead-values pass in MLIR upstream) after moving attributes back to operations.

[darthscsi]

You cannot safely propagate unknown attributes.
https://discourse.llvm.org/t/on-querying-an-operations-intrinsic-core-vs-external-user-defined-attributes/4498/5

[darthscsi]

Cannot be done safely and has been discussed many times over the course of the past couple years.
https://discourse.llvm.org/t/on-querying-an-operations-intrinsic-core-vs-external-user-defined-attributes/4498/5

[7FM]

Thanks for the feedback! I will close this PR and the corresponding issue for now.