[PATCH] D79676: [Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to OMPBuilderCBHelpers

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:878
+              [this, VDInit, OriginalAddr, VD, ThisFirstprivateIsLastprivate,
+               OrigVD, &Lastprivates, IRef, &OMPBuilder]() {
                 // Emit private VarDecl with copy init.
----------------
Is this needed?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1549
+  // region body, and finalization codegen callbacks. This will class will also
+  // contain privatization functions used by the privatization call backs
+  struct OMPBuilderCBHelpers {
----------------
Nit: Make it a doxygen comment with 3x`/`, -`will` + full stop.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1561
+
+    private:
+      llvm::CallInst *RTLFnCI;
----------------
Nit: No need.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1581
+        ++I;
+
+      return &*I;
----------------
What is wrong with `BB->getFirstNonPHIOrDbgOrLifetime()`? If it is to avoid too many test changes, please add a TODO. I think from a functional perspective there is no need to advance it after the first set of allocas that may be separated from the phis by non-allocas?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79676