ftynse added a comment.
Thanks! I have a couple of comments, but I will defer to @jdoerfert for
approval in any case.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:676
+
+ /// Capture the above-defined paraneters for the parallel regions.
+ ///
----------------
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:678
+ ///
+ /// \param CaptureAllocaInsPoint Insertion point for the alloca-ed struct.
+ /// \param OuterFn The function containing the omp::Parallel.
----------------
Nit: it looks like this file uses IP rather than InsPoint for names related to
insertion points
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:760
+ SetVector<BasicBlock *> BlockParents;
+ for (unsigned Counter = 0; Counter < Blocks.size(); Counter++) {
+ BasicBlock *ParallelRegionBlock = Blocks[Counter];
----------------
Nit: I think
https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
applies to `.size()` the same way it applies to `.end()`
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:795
+ // captured values with the extracted ones from the struct.
+ if (CapturedValues.size()) {
+ // Create the StructTy
----------------
Nit:
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
```
if (CapturedValues.empty())
return;
```
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:796
+ if (CapturedValues.size()) {
+ // Create the StructTy
+ std::vector<Type *> StructTypes;
----------------
Nit: trailing dot
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:799
+ for (unsigned Counter = 0; Counter < CapturedValues.size(); Counter++)
+ StructTypes.push_back(CapturedValues[Counter]->getType());
+
----------------
Nit: please `reserve` before pushing back in a loop
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:807-808
+ llvm::IRBuilder<>::InsertPointGuard Guard(Builder);
+ Builder.SetInsertPoint(CaptureAllocaInsPoint.getBlock(),
+ CaptureAllocaInsPoint.getPoint());
+ // Allocate and populate the capture struct.
----------------
Nit: `Builder.restoreIP(CaptureAllocaInsPoint)` looks shorter
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:810-815
+ AllocaInst = Builder.CreateAlloca(CaptureStructType, nullptr,
+ "CaptureStructAlloca");
+ Value *InsertValue = UndefValue::get(CaptureStructType);
+ for (auto SrcIdx : enumerate(CapturedValues))
+ InsertValue = Builder.CreateInsertValue(InsertValue, SrcIdx.value(),
+ SrcIdx.index());
----------------
I suppose you may want to have `alloca` inserted in a block (function entry)
different from the one where you store into the memory. You need to store just
before calling the fork function (or, at least, so that the store postdominates
all stored values). Looking at the function API, I would have assumed
`CaptureAllocaInsPoint` to be an insertion point at the function entry block
specifically for `alloca`s, where these `insertvalue`s are invalid.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:826-829
+ for (unsigned Counter = 0; Counter < Blocks.size(); Counter++)
+ for (auto I = Blocks[Counter]->begin(), E = Blocks[Counter]->end();
+ I != E; ++I)
+ for (Use &U : I->operands())
----------------
Can we rather take each captured value and enumerate its uses, replacing those
within the parallel block set?
================
Comment at: llvm/test/CodeGen/XCore/threads.ll:84-140
+; This test fails on Windows because the second and third
+; register of the add operations are swapped.
+; Windows test is below.
+; REQUIRES: !windows
define i32* @f_tle() {
; CHECK-LABEL: f_tle:
; CHECK: get r11, id
----------------
These look irrelevant to the patch, but seem to fix a breakage upstream. Would
you mind committing this separately?
================
Comment at: mlir/test/Conversion/OpenMPToLLVM/openmp_float-parallel_param.mlir:1
+// RUN: mlir-translate --mlir-to-llvmir %s | FileCheck %s
+
----------------
Changes to MLIR are no longer necessary
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
