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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

This seems sensible. The clang parts look OK but I haven't thought everything through to the last detail. I left some questions below, I guess we can go down this road and slowly transition to the new scheme.



================
Comment at: clang/lib/AST/Stmt.cpp:1275
-                                                ->isScalarType())) &&
-        "captures by copy are expected to have a scalar type!");
     break;
----------------
Why does this have to go? Is that avoidable?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17274
+  if (IsTopScope && Kind != Sema::TryCapture_Implicit) {
+    ByRef = (Kind == Sema::TryCapture_ExplicitByRef);
+  } else if (S.getLangOpts().OpenMP && RSI->CapRegionKind == CR_OpenMP) {
----------------
This doesn't impact anyone else negatively? I don't understand why we need this now.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:305
 
+  /// Modifies the canonical loop to be a workshare loop.
+  CanonicalLoopInfo *createWorkshareLoop(const LocationDescription &Loc,
----------------
Nit: Add the argument description (from above).


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:865
 
+  Function *getFunction() const { return Header->getParent(); }
+
----------------
Nit: documentation, also above.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:986
+  if (BodyGenCB)
+    BodyGenCB(CL->getBodyIP(), CL->getIndVar());
 
----------------
I'm unsure I understand when it would make sense to not have a body generator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973