[Mlir-commits] [mlir] [mlir] Enable LICM for ops with only read side effects in scf.for (PR #120302)

[Scott Manley]

rscottmanley wrote:

I have some issues with this merge, while I appreciate the idea behind it. 

Namely, even if a custom `shouldMoveOutOfRegion()` is provided, that decision is effectively overridden by
```
if (!loopSideEffectFreeOrHasOnlyReadSideEffect && !isMemoryEffectFree(op)) {
```
It seems like like `shouldMoveOutOfRegion()` should contain this logic instead in the default implementation so downstream users can override it with their own analysis -- especially needed for custom Dialects/Ops. I think this probably also needs to return HOW it should move out of the region so it can be under guard or not (I think there is a broader discussion on the issues of top test with SCF loops but that's another topic)

Also, `moveOutOfLoopWithGuard()` default implementation is simply a return? So nothing is done? This seems incomplete based on the description of the PR? Am I missing something?

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