[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:292
+  /// nest would extend.
+  SmallVector<llvm::CanonicalLoopInfo *, 4> OMPLoopNestStack;
+
----------------
jdenny wrote:
> Unless I missed something, the only accesses to `OMPLoopNestStack` are `push_back`, `clear`, `back`, and `size`.  It's never popped.  That is, every push appears to mean that all existing elements will never be accessed again.  So why is it a stack?  Is that purely for the sake of the assertion that calls `size`?
When eventually supporting loop nests, we will need not only the outermost loop, but also inner ones.


================
Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform<Derived>::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the loop-associted
+  // directive.
----------------
jdenny wrote:
> I'm used to seeing `TransformX` call `RebuildX` call `ActOnX`.  Why not do that for `X=OMPCanonicalLoop`?  Does `TransformOMPExecutableDirective` really need a special case for `OMPCanonicalLoop`?
The intended behaviour is: 

1. Transform the child loop
2. Return the child loop as representing the OMPCanonicalLoop (i.e. OMPCanonicalLoop wrapper is removed).
3. Parent loop-associated directive (e.g. workshare) is processed.
4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for loop nest.

This guarantees maximum flexibility on what of the loop can be changed, such as
* Change lower bound, upper bound, step
* Convert between CXXForRangeStmt and ForStmt
* Change the associated depth (e.g. different value for `collapse` clause)
* Remove the directive and no OMPCanonicalLoop remain

This also saves adding many lines of code handling transforming each member of OMPCanonicalLoop separately.


================
Comment at: clang/tools/libclang/CIndex.cpp:2839
+  VisitStmt(L);
+  EnqueueChildren(L);
+}
----------------
jdenny wrote:
> Doesn't `VisitStmt(L)` already call `EnqueueChildren(L)`?
It does indeed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94973/new/

https://reviews.llvm.org/D94973