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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur marked 6 inline comments as done.
Meinersbur added a subscriber: rsmith.
Meinersbur added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:94
   // been "emitted" to the outside, thus, modifications are still sensible.
-  if (CGM.getLangOpts().OpenMPIRBuilder)
-    CGM.getOpenMPRuntime().getOMPBuilder().finalize();
+  if (CGM.getLangOpts().OpenMPIRBuilder && CurFn)
+    CGM.getOpenMPRuntime().getOMPBuilder().finalize(CurFn);
----------------
jdenny wrote:
> Without this patch, `finalize` is called even if `!CurFn`?
Without CurFn, there is no function to finalize.

The problem before this patch is that finalize would finalize ALL functions, even those that are still in construction. In particular, finalizing the Distance or LoopVar function would also finalize the parent function.

I added an assert for OpenMPIRBuilder's dtor that checks that everything has eventually been finalized.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3771
+
+  void VisitOMPCanonicalLoop(OMPCanonicalLoop *S) { VisitStmt(S); }
+
----------------
jdenny wrote:
> Isn't `VisitStmt` called for any `Stmt` without this?
I think you are correct.


================
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> {
----------------
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.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+      Expr *NegIncAmount =
+          AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+      Expr *BackwardDist = AssertSuccess(
----------------
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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973