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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
