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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
