[clang] [llvm] [mlir] [openmp] [LoopTiling][Clang][OpenMP] Canonical Intra-tile Loops (PR #191114)

[Amit Tiwari via cfe-commits]

loopacino wrote:

> That is, without additional code in CGStmt that catches the AttributedStmt, a predicate is emitted. I would have done it the other way around: Make the default AST emit the vectorizable code (so no additional handling in CGStmt is needed; AttributedStmt can just be ignored, no change in output), and make `checkOpenMPLoop` recognize the AttributedStmt: If present, it knows it can convert the non-canonical ForStmt condition into a canonical one by remembering it still has to emit a predicate around the body in TransformedStmt. The main benefit is that this is more localized to the handling of OpenMP loops than dragging its complexity into the general-purpose CodeGen. Downside might be that the predicate needs to be remembered through the composition of multiple loop transformations (altough I think this approach might have the same problem; tests for it are missing). This is not how it needs to be done, but I would like to know why not.


After spending some time comparing the two, few points came up. Initially, it seemed that the current (canonical by-default) is safer "if any future directive misses out the marker". It turns out that in both the approaches, there exist a failing case if the marker is missed/unrecognized. In the current approach, the inner-loop is re-emitted and the loop runs too many times (was also a bug handled before posting the recent changes). In the min-bound default, it is obvious to see predicate not getting emitted resulting in incorrect semantics. So, either way, we rely on the marker to be handled properly.

In the min-bound by-default, if we consider the case where the tiled loop isn't consumed–results in not touching the CodeGen. However, min-bound by-default wouldn't allow us to remove CodeGen impact altogether, especially in case where we would have to emit the predicate later; also unfolding min-bound later is less convenient than pushing predicate to min-bound (in canonical by-default, not a deal-breaker I agree). The point is that both the designs need that emit-side handling. So the "no CodeGen change" benefit is real, but it only covers the standalone case, not the consuming cases.

Keeping the canonicalized by-default seems to be a valid-rectangular, self-consistent (transformed AST) form in every context–standalone, 2-level tiling, <some-directive>-nesting tiling, etc. I would rather keep the always-valid form. If keeping OpenMP checks into `EmitForStmt` is a main concern, we may reconsider the emitting order from canonical by-default to min-bound by-default. Let me know your opinions.

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