[Mlir-commits] [mlir] [mlir][Transforms] Dialect conversion: Fix bug in `UnresolvedMaterializationRewrite` rollback (PR #105949)

[Matthias Springer]

matthias-springer wrote:

> This seems fine to me, as in being a step in the right direction, but it doesn't seem guaranteed to me that the input operand was actually the operand that was originally mapped to the materilization:
> 
> https://github.com/llvm/llvm-project/blob/0b9a69b3beb03f0de9b3c699e9b32a7b61b3ca57/mlir/lib/Transforms/Utils/DialectConversion.cpp#L1155
> 
> That said, this seems to be more of a pre-existing issue to me.

You are right, it's not a perfect rollback and there are probably other issues that are not fixed by this PR.

It's actually not just that line of code that you pointed out. Here's another one: https://github.com/llvm/llvm-project/blob/0b9a69b3beb03f0de9b3c699e9b32a7b61b3ca57/mlir/lib/Transforms/Utils/DialectConversion.cpp#L2604

I think this is due to the fact that:
- An SSA value may be converted to multiple different types. E.g., different patterns with different type converters may create multiple conversions of the same SSA value to different types during `remapValues`. (And we want to convert the original value, not an already converted value.)
- The `mapping` data structure is a "flat" `IRMapping`. An SSA value can be mapped only to one other SSA value. We emulate the fact that there may be multiple conversions by chaining mappings. E.g., the first conversion will map to the second conversion. Kind of like a linked list. When we should really have a mapping of `Value` to `SmallVector<Value>`.

I'm thinking about fixing that. But not sure if it's worth it, because ultimately I want to get to a state where all IR changes are materialized immediately, and there is no need for a `mapping` anymore.

(The purpose of this change was mostly to cut down on code size. Bonus points if it fixes something. Anything that reduces complexity and the lines of code will make it easier to maintain the code base and/or build a new conversion driver.)


https://github.com/llvm/llvm-project/pull/105949