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

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:292
+  /// nest would extend.
+  SmallVector<llvm::CanonicalLoopInfo *, 4> OMPLoopNestStack;
+
----------------
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`?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5355
+  Expr *Cond, *Inc;
+  VarDecl *CounterDecl, *LVDecl;
+  if (auto *For = dyn_cast<ForStmt>(AStmt)) {
----------------
`CounterDecl` is the declaration of the "loop iteration variable" based on the comments now on `OMPCanonicalLoop`, right?  If so, can we update the variable names here?  One possibility is `LIVDecl` and `LUVDecl`.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5139
+/// Used to capture variable references if already parsed statements/expressions
+/// into a CapturedStatement.
+class CaptureVars : public TreeTransform<CaptureVars> {
----------------
Meinersbur wrote:
> jdenny wrote:
> > I think this means:
> > 
> > > If already parsed statements/expressions, used to capture variable references into a CapturedStatement.
> > 
> > But it reads like it means:
> > 
> > > If already parsed statements/expressions into a CapturedStatement, used to capture variable references.
> if -> of
> 
> I reformulated the sentence which I hope is now more clear.
That's better.  Thanks.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+      Expr *NegIncAmount =
+          AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+      Expr *BackwardDist = AssertSuccess(
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > jdenny wrote:
> > > It looks like the AST nodes `NewStart`, `NewStop`, and `NewStep` are shared by multiple parent nodes here.  Does that ever happen anywhere else in a Clang AST?
> > Yes. For instance, when instantiating a template, nodes form the TemplatedDecl are reused unless they need to change. Therefore multiple template instantiation can also share entire AST subtrees. Why rebuild them if they are identical?
> > 
> > However, I also found the following description of TreeTransform::AlwaysRebuild():
> > ```
> >   /// We must always rebuild all AST nodes when performing variadic template
> >   /// pack expansion, in order to avoid violating the AST invariant that each
> >   /// statement node appears at most once in its containing declaration.
> > ```
> > It was added by @rsmith in rG2aa81a718e64fb4ca651ea12ab7afaeffc11e463 to fix llvm.org/PR17800 . The assertion doesn't seem to exist in the current code base.
> > 
> > Does this apply here? 
> I think the invariant exists because codegen has a map from ast expression nodes to its temporary memory location. This means that reusing the same AST expression node indeed should not be done.
> 
> I think the assertion that failed in llvm.org/PR17800 is now here:
> https://github.com/llvm/llvm-project/blob/7ba2e1c6011eeb1b91ce3f5d8fa7187b7518e77a/clang/lib/AST/ExprConstant.cpp#L1872
> 
> Assertion message was changed in this commit:
> https://github.com/llvm/llvm-project/commit/f7f2e4261a98b2da519d58e7f6794b013cda7a4b
Thanks for the links.

I guess reuse is ok across different instantiations of a template because this map is cleared across different functions.

It might not be worth much at this point, but I also saw this comment as more evidence for this invariant: [[ https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#ad6ff7b541a77cea65d8f72ed3f903fb3 | "expression's are never shared"]].


================
Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform<Derived>::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the loop-associted
+  // directive.
----------------
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`?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973