[Comb][Canonicalize] keep attributes during op width narrowing

[7FM]

Fixes #5531.
TLDR: narrowOperationWidth() copies attributes to a comb.concat operation, which is optimized away in a later step.
The fix is simple: do not copy the attributes to the intermediate comb.concat op but instead to the width narrowed operation.

[uenoku]

One tricky issue here is that strictly speaking %2 after canonicalization is not equal to %3. Actually it makes sense to me to propagate names even when they are not accurate, but we should also consider deriving names from original namehints, such as hint_0_to_32 for this example.

[7FM]

One tricky issue here is that strictly speaking %2 after canonicalization is not equal to %3.

When considering the possibility of overflows, then technically the same could be said about the previous behavior with %newop = comb.concat %c0_i32, %mul_32_res {sv.namehint = "hint"} : i32, i32 :D
Also, it is a different operation type altogether that gets the name hint (before being optimized away).
But I agree that it is not perfectly accurate.

The reason that I am bringing this up, is that we (@jopperm) use custom annotations for some comb operations in our out-of-tree workflow, and it would be great if they would still exist after -canonicalize :)

[uenoku]

Yeah, preserving optional attributes is hard problem and MLIR doesn't provide a good solution for it (Links for previous discussion on forums: https://discourse.llvm.org/t/canonicalization-passes-dont-keep-attributes/59750/2). Currently we treat sv.namhint specially and are manually propagating hints, and other optional attributes are not propagated at all. I think it would be unsafe to propagate optional attributes freely.

[7FM]

Yeah, preserving optional attributes is hard problem and MLIR doesn't provide a good solution for it

Too bad, anyway, thanks for the link.

other optional attributes are not propagated at all.

Is this a comb specific decision? At least in other dialects, I have seen multiple occasions with newOp->setDialectAttrs(op->getDialectAttrs()); in their lowering passes.

I think it would be unsafe to propagate optional attributes freely.

Agreed, this is very much dependent on the intended semantics of the attributes.

[teqdruid]

@uenoku In general, dialect attributes should be propagated best effort, so I think it's appropriate to propagated them through the canonicalizers where it's obvious what the destination/source op is. If that's unsafe in some way, it's the dialect's responsibility to detect that situation, IMO.

[uenoku]

Is this a comb specific decision? At least in other dialects, I have seen multiple occasions with newOp->setDialectAttrs(op->getDialectAttrs()); in their lowering passes.

Yeah, good point. I kind of feel it's generally safe to use newOp->setDialectAttrs(op->getDialectAttrs()) in lowering passes, since in that case there is some direct mapping from source to target operations (e.g. seq.reg -> sv.reg, hwarith.add -> comb.add). But I feel it's too aggressive to propagate all unknown attributes in canonicalizers since I guess attributes could be moved to random operations.

In general, dialect attributes should be propagated best effort, so I think it's appropriate to propagated them through the canonicalizers where it's obvious what the destination/source op is. If that's unsafe in some way, it's the dialect's responsibility to detect that situation, IMO.

I agree that it's ok to propagate attributes when the dest/src are clear.

[teqdruid]

Yeah, all attributes is definitely not safe. That's what dialect attributes are for.

[7FM]

I agree that it's ok to propagate attributes when the dest/src are clear.

So it should be fine here? Both op and newOp are of the template type OpTy.

[uenoku]

Yeah, probably. It's still a bit concerning that types are different though. Any thought? @darthscsi

[darthscsi]

When considering the possibility of overflows, then technically the same could be said about the previous behavior with %newop = comb.concat %c0_i32, %mul_32_res {sv.namehint = "hint"} : i32, i32 :D

Yea, it's moving the result of the later extract (in this case) into the visibility of mul's value. On the firrtl side, we would not have propagated a name in this case. (Actually, we would have rooted the pattern at the extract and preserved its name)

I don't know if there is a general rule we can depend on, but it does seem like width-narrowing is a case where propagating the dialect attributes makes sense.

[darthscsi]

Width narrowing seems a clear instance where best-effort means moving the dialect attributes. As pointed out, the existing pattern already was changing the visible value for a particular name, so this is not obviously worse.