[PATCH] D91556: Add capturing of parameters to pass to omp::parallel

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think the overall approach is good to solve the problem with max 16 arguments. Is this based on current master? I left remarks wrt style and other things below, I will need to go over the logic again after those are addressed.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:627
+      const SmallVector<BasicBlock *, 32> &Blocks, const Value *const TIDAddr,
+      const Value *const ZeroAddr);
 };
----------------
Nit: use SmallVectorImpl w/o the size. No const on the pointers (*const); doesn't mean much anyway.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:446
 
+  Instruction *ThreadIDInst = dyn_cast<Instruction>(ThreadID);
+
----------------
If it has to be an instruction, use `cast`. If it might no be, simply get the current instruction point from the builder.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:698
+  captureParallelRegionParameters(CaptureAllocaInsPoint, OuterFn, Blocks,
+                                  TIDAddr, ZeroAddr);
+
----------------
This should make some of the code introduced by D92189 obsolete, right?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:844
+  }
+}
+
----------------
Use range loops above whenever possible. Use LLVM naming style for variables please, so first letter capitalized. Also no `llvm::`.
Prefer Insertion point guards over manually saving restoring (potentially adding a explicit scope `{ ... }`).
I don't think we need to iterate over the entire set of instructions of the outer function, do we? Check how D92189 identifies communicated values.


================
Comment at: mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp:17
+
+#include "llvm/ADT/SetVector.h"
 
----------------
leftover.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91556