[Mlir-commits] [mlir] [mlir][reducer] Make mlir-reduce don't delete terminator and use ub.poison to repalce op's results (PR #185445)

[lonely eagle]

linuxlonelyeagle wrote:

> I was requested for review, but I'm not sure I have much to add here, though some questions may help my familiarity with this codebase so I can review more in the future:

Thank you so much for reviewing the code and providing such valuable suggestions and questions. I believe it's important for us to think through these issues carefully.

> * What is the purpose of the "ops in range" concept?
”ops in range” represents the subset of IR within a region, identified by the algorithm, that must be preserved; all other IR should be removed.(Of course, I don't believe all IR must be removed.)

> *The need to remove ops not in the given range seems to lead to this conflicting situation where you can't properly remove the op when it mixes with control flow.

Actually, I've already been considering the data flow issues. I believe we can handle control flow scenarios by preserving the RegionBranchTerminatorOpInterface, which should ensure data flow correctness. I just haven't implemented this part yet.

> * 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.)

Yes, may be we use `builtin.unrealized_conversion_cast` better.

> Then again, I would think that removing that `memref.alloc` just shouldn't be in scope of what this reduction should be doing. Probably better would be to have a pattern that knows about successors and can, e.g., remove an entire block and point predecessors to a different block in the region, which would circumvent this problem...

Can I interpret it this way: if all ops in a block are removed, I can implement a pattern to delete the block itself. Additionally, we should locate its predecessors and redirect their successors to other blocks. I think this feature is quite exciting.

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