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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

In D104928#2844722 <https://reviews.llvm.org/D104928#2844722>, @Meinersbur wrote:

> `createReductions` takes many arrays. How feasible is it to make it emit just one reduction per call, so front-ends would call it once per reduction?

I expect this to double the complexity and triple the length of the code. One reduction clause (with potentially several reductions in it) is mapped to one runtime call, which takes an array of pointers to reduction variables. Adding reductions progressively to that requires to modify many details in the IR, in particular the size of allocas and the signature of the outlined function, among other things. On the other hand, I expect the list of reductions to be fully available when processing a directive. I can propose using some ReductionInfo struct that contains all the relevant input pieces and have one array instead of four co-indexed arrays.

> In the current form, would it be even legal to it multiple times with just one variable to reduce?

This will produce two reduction structures with two synchronization points. They will use the same critical section lock so they wouldn't deadlock, but this may not be the expected semantics.



================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2749
+  GlobalVariable *LockVar =
+      M->getGlobalVariable(".gomp_critical_user_.reduction.var");
+  EXPECT_NE(LockVar, nullptr);
----------------
Meinersbur wrote:
> Why `gomp` ("GNU OpenMP") in the name?
This is the name emitted by the builder for critical section variables - https://github.com/llvm/llvm-project/blob/dc4299a7f3ad7e4fa3c310d585de4e46bde58d16/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L2238 - I haven't chosen it. I suppose a compatibility layer, potentially outdated, for the emitted code to work across libopenmp and libgomp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104928