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

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

jdenny added inline comments.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:166
+    assert((isa<ForStmt>(S) || isa<CXXForRangeStmt>(S)) &&
+           "Canonical loop must be a for loop (range-based or otherwise)");
+    SubStmts[LOOPY_STMT] = S;
----------------
Meinersbur wrote:
> jdenny wrote:
> > To convert run-time errors into compile-time errors, what if `setLoopStmt` is overloaded to take either a `ForStmt` or `CXXForRangeStmt` instead of any `Stmt`?
> This would also require callers to call two different versions and just removes the problem up the call stack. For instance, StmtReader just receives a Stmt from ReadStmt. If we had setLoopStmt, we'd need a switch to call one of overloads.
OK, I checked the call stack for your setters this time.  It looks like it's just `create`, and I see that trying to achieve my goal would make a mess of that call stack.  Sorry for the noise.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1872
+  LValue CapStruct = ParentCGF.InitCapturedStruct(*S);
+  CodeGenFunction CGF(ParentCGF.CGM, true);
+  std::unique_ptr<CodeGenFunction::CGCapturedStmtInfo> CSI =
----------------



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3497
+
+      bool NeedsBarrer = !S.getSingleClause<OMPNowaitClause>();
+      llvm::OpenMPIRBuilder &OMPBuilder =
----------------



================
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);
----------------
Without this patch, `finalize` is called even if `!CurFn`?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:288
+  /// this stack when done. Entering a new loop requires clearing this list; it
+  /// either means we start parsing an new loop nest (in which case the previous
+  /// loop nest goes out of scope) or a second loop in the same level in which
----------------



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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973