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

[Alex Zinenko via Phabricator via llvm-commits]

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