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

[Lubomir Litchev via Phabricator via llvm-commits]

llitchev added inline comments.


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


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:446
 
+  Instruction *ThreadIDInst = dyn_cast<Instruction>(ThreadID);
+
----------------
jdoerfert wrote:
> If it has to be an instruction, use `cast`. If it might no be, simply get the current instruction point from the builder.
Using the current position from the Builder (moving one forward, so it is not the end() ).


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:698
+  captureParallelRegionParameters(CaptureAllocaInsPoint, OuterFn, Blocks,
+                                  TIDAddr, ZeroAddr);
+
----------------
jdoerfert wrote:
> This should make some of the code introduced by D92189 obsolete, right?
Yes! Most of it. Now the data is just wrapped into a capture struct before calling the synthetic ..._fork function.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:844
+  }
+}
+
----------------
jdoerfert wrote:
> 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.
You are right ... That was one of the changes I wanted to make to the Diff. Now iterate over the parallel region blocks only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91556