[PATCH] D91556: [OpenMPIRBuilder} 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:119
   ///                  should be placed.
-  /// \param Original The value being copied/created, should not be used in the
-  ///                 generated IR.
-  /// \param Inner The equivalent of \p Original that should be used in the
-  ///              generated IR; this is equal to \p Original if the value is
-  ///              a pointer and can thus be passed directly, otherwise it is
-  ///              an equivalent but different value.
+  /// \param \param Val The value beeing copied/created.
   /// \param ReplVal The replacement value, thus a copy or new created version
----------------
jdoerfert wrote:
> wasn't that a spelling error before? and one `\param` is enough ;)
Fixed.


================
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.
----------------
ftynse wrote:
> Nit: it looks like this file uses IP rather than InsPoint for names related to insertion points
No need to store this value anymore. Used the InsertBB->getTerminator(), thus guaranteeing the alloca and stores are just before the fork call (they were before that call too, since the ThreadID was called last), so even if more codegen is introduced in the future the logic deals with it.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:441
+  IRBuilder<>::InsertPoint CaptureAllocaInsPoint(Builder.GetInsertBlock(),
+                                                 --Builder.GetInsertPoint());
+
----------------
jdoerfert wrote:
> How do we know that `--` is valid here? Couldn't `Loc` point to the begin of a function? If possible, let's just use `Loc.IP`.
There is always an instruction before - the ThreadID was always generated, and that is what -- points to. Changed it to use InsertBB->getTerminator(). It is much more sturdy this way. Even if the codegen is changed, the alloca, insert and store will be generated always just before the forkCall.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:760
+  SetVector<BasicBlock *> BlockParents;
+  for (unsigned Counter = 0; Counter < Blocks.size(); Counter++) {
+    BasicBlock *ParallelRegionBlock = Blocks[Counter];
----------------
ftynse wrote:
> 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()`
Fixed.


================
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.
----------------
ftynse wrote:
> Nit: `Builder.restoreIP(CaptureAllocaInsPoint)` looks shorter
Refactored. No need to store the InsertPoint.


================
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());
----------------
ftynse wrote:
> 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.
Now it is guaranteed that. the codegen of the alloca, insert, and stores are done just before the forkCall. Even if the codegen changes in the future. It was the case before because the code was generated after the ThreadID getting call (which was just before the fork).


================
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())
----------------
ftynse wrote:
> Can we rather take each captured value and enumerate its uses, replacing those within the parallel block set?
That was the first implementation I had. The issues was that the uses() was not returning all the uses (particularly the ones introduced by the loop unroller - spent bunch of time debugging it). Iterating to all the instruction parameters of the parallelRegions just works.


================
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
----------------
ftynse wrote:
> These look irrelevant to the patch, but seem to fix a breakage upstream. Would you mind committing this separately?
OK


================
Comment at: mlir/test/Conversion/OpenMPToLLVM/openmp_float-parallel_param.mlir:1
+// RUN: mlir-translate  --mlir-to-llvmir %s | FileCheck %s
+
----------------
ftynse wrote:
> Changes to MLIR are no longer necessary
Yes. This just exposes the original issue I had. I thought it is useful to have a test that verifies the underlined functionality works for MLIR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91556