[Mlir-commits] [mlir] [mlir][reducer] Introduce the materialization mechanism in the reduction-tree and fix the logic for deleting operations (PR #185445)

[lonely eagle]

linuxlonelyeagle wrote:

> * The idea that removing an op = replacing it with poison seems a bit suspicious. It seems like it's added here as a default when no other reasonable value can be materialized. I suspect that `builtin.unrealized_conversion_cast` would be closer semantically to what you want, and it can support 0-1 or even 0-to-N conversions which would allow you to bridge together any number of block args that might be lost from removed ops. (e.g., what if you remove an op that has multiple results? `poison` has a single result.)

While investigating this issue, I encountered several problems.
The first problem is that the builtin.unrealized_conversion_cast op is too long, which means the IR after materialization may be longer. An example of materialization failure due to this can be seen in the PR.
The second problem is that in the PR you can see now, the builtin.unrealized_conversion_cast op has no operands, because we deleted part of the IR. This means that an op which originally had users might end up with no users, and could then be incorrectly deleted. So I think this part still needs careful consideration — we should approach this conservatively.
The third problem is that even if we do properly set the operands, bugs may still arise. For example, builtin.unrealized_conversion_cast i32 to i32 — if the operand and result are the same type, the cast will be eliminated. But here the cast is only serving as an IR barrier to preserve IR correctness; it should not be eliminated.

Taking all of the above into account: I plan to add a new op whose serialized size should be smaller than any existing op (the tester measures IR size by the size written to file), and which introduces no additional side effects. Do you have any good ideas for the design of this op — for instance, what should it be named? cc: @j2kun 

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