[PATCH] D104928: [OMPIRBuilder] add minimalist reduction support

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM. Just some requests for additional comments.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:517
+
+    /// Callback for generating the atomic reduction body, may be null.
+    AtomicReductionGenTy AtomicReductionGen;
----------------
Could you add a short explanation about the relation between `ReductionGen` and `AtomicReductionGen`, and what happens if the latter is null?


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2993-2998
+  EXPECT_NE(M->getFunction("func..omp_par"), nullptr);
+  EXPECT_NE(M->getFunction("func..omp_par.2"), nullptr);
+
+  // Two different reduction functions must have been created.
+  Function *AddReduction = M->getFunction(".omp.reduction.func");
+  Function *XorReduction = M->getFunction(".omp.reduction.func.1");
----------------
Not a request to change, but it might have been more robust to look for the runtimes calls that these are passed to that relying on how they are named.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2620-2621
+    Builder.restoreIP(OuterAllocaIP);
+    SumReduced = Builder.CreateAlloca(Builder.getFloatTy());
+    XorReduced = Builder.CreateAlloca(Builder.getInt32Ty());
+  }
----------------
Could you give these a name just such they are easier to identifier in the dump?


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2625-2626
+  // Store initial values of reductions into global variables.
+  Builder.CreateStore(ConstantFP::get(Builder.getFloatTy(), 0.0), SumReduced);
+  Builder.CreateStore(Builder.getInt32(1), XorReduced);
+
----------------
ftynse wrote:
> Meinersbur wrote:
> > Could you give these a name just such they are easier to identifier in the dump?
> We can't set names on store instructions, can we?
😉


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2695-2710
+  auto SumReduction = [&](InsertPointTy IP, Value *LHS, Value *RHS,
+                          Value *&Result) {
+    IRBuilderBase::InsertPointGuard Guard(Builder);
+    Builder.restoreIP(IP);
+    Result = Builder.CreateFAdd(LHS, RHS, "red.add");
+    return Builder.saveIP();
+  };
----------------
ftynse wrote:
> Meinersbur wrote:
> > Should OMPIRBuilder provide a set of standard reduction callbacks so users don't have to declare them?
> Would be nice to have, but I'd prefer not to scope creep this patch.
Could you add a TODO then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104928