[flang-commits] [flang] [mlir] [mlir][CSE] Introduce hoist-pure-ops logic to CSE pass (PR #180556)

[lonely eagle via flang-commits]

linuxlonelyeagle wrote:

> CSE has `markAnalysesPreserved<DominanceInfo, PostDominanceInfo>()`, is this still correct? Before we were just deleting redundant expression, now we're moving across blocks which likely invalidate it? We should detect when operations move across blocks and not preserve the analysis when it happens (or update it?).

Let me address this question now, `markAnalysesPreserved<DominanceInfo, PostDominanceInfo>()` remains correct.
You can see https://github.com/llvm/llvm-project/blob/682ae8b626393ad77afebebda912f14f3c55a003/mlir/lib/IR/Dominance.cpp#L299
We only rely on the Dominator Tree when determining dominance relationships between different blocks within the same region. In all other cases, dominance can be inferred from the IR's relative positions. The implementation of DominanceInfo leverages LLVM's DominatorTreeBase, which aligns perfectly with LLVM IR's design. This means that although I am hoisting operations here, it does not fundamentally change the relative positioning of blocks within the region. Therefore, DominanceInfo and PostDominanceInfo remain valid.

> Also I'm not sure about test coverage, you had to update many tests, but it's not clear that we cover all the edge cases. Do we have tests covering hoisting blocked at IsIsolatedFromAbove in various conditions? I would suggest adding new minimal tests specifically targeted for this CSE hoisting.

I’ve added some test cases and would love to hear your thoughts. I’m definitely looking for more feedback on this part. 😉. 

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