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