[Mlir-commits] [mlir] [mlir][Transforms] CSE: Add filter options to control CSE'ing (PR #115639)

[River Riddle]

River707 wrote:

> Some context: In my use case, certain ops with regions should be a hoisting barrier for certain nested ops. CSE is one example of hoisting (by de-deduplication). Another one is the `GreedyPatternRewriteDriver`, which hoists constants.
> 
> I ran into this issue in the past when I was working on a transform dialect-based experiment in IREE: Ops without tensor results were allowed to be hoisted from a "dispatch region" op, but not ops with tensor results.
> 
> I just took a look at the `DialectFoldInterface` interface. It seems related but does not quite fit. The place where a constant is materialized is defined by the folded op (or its dialect to be precise). In my use case, it looks like I'd need the property (of being a hoisting barrier) on the region op, not the op that is going to be hoisted.

I think there is confusion here, that is not what the materialization hook does. That hook is used exactly as you describe your need: it's defined by dialects with region operations that want to define hoist barriers, or more directly region operations that constants should or shouldn't be materialized above. If you look at the logic where this is used, the insertion region is determined by walking upwards from the insertion block to find a suitable region location (using the parent operations as the hooks for the interfaces): https://github.com/llvm/llvm-project/blob/5082acce4fd3646d5760c02b2c21d9cd2a1d7130/mlir/lib/Transforms/Utils/FoldUtils.cpp#L38 

> 
> What could work is a `HoistingBarrierOpInterface` with an interface method `bool isBarrierFor(Operation *op)` with a default implementation of `return false`. In my use case, the region op would implement the interface and return "true" if `op` has a tensor result.
> 
> CSE and the greedy pattern driver could query that interface. Any thoughts?

Yes, I think something like this could be a good idea. I would hope though, that this would replace the shouldMaterializeInto hook (I don't know of any cases that don't map to hoisting more generally). I could be wrong though, but I haven't seen such a case in practice.

nit on the default: I would expect a barrier op interface to default to being a barrier for everything (not the other way around).



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