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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:784
+    }
+  }
+
----------------
1) If we would need this, remove the Counter stuff everywhere, if you want to iterate a container: `for (const T& : Container)`
2) `BlockParents` seems to be a set with the blocks, we already have that, it's called `ParallelRegionBlockSet`, simply pass it in.
3) Why don't we use the `Inputs` and `Outputs` set computed by the `findInputsOutputs` call. Those are the live-in and live-out values of the parallel region.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:795
+  unsigned CapturedSize = CapturedValues.size();
+  std::vector<Type *> StructTypes;
+  StructTypes.reserve(CapturedSize);
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:806
+    llvm::IRBuilder<>::InsertPointGuard Guard(Builder);
+    Builder.SetInsertPoint(InsertBeforeInst);
+
----------------
The alloca needs to go in the `OuterAllocaIP` passed in by the caller of `CreateParallel`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:821
+    Value *LoadedValue =
+        Builder.CreateExtractValue(LoadedAlloca, SrcIdx.index());
+
----------------
I'm not too happy with this insert/extract value scheme. Without further optimization (-O0) this might not be lowered properly. Why don't we create a GEP and load/store to the appropriate location instead?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:831
+            U.set(LoadedValue);
+  }
+}
----------------
Instead of doing this, unpack/load the location in the `PrivHelper` like we did before. Also, pass the loaded value as `Inner` to the `PrivCB` so that the callback has both the original value `V` and the reload `Inner`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91556