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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Sorry for the time, I should look for reviews more often. I applied the patch locally and will give more feedback.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:572-575
+                   ArrayRef<Value *> Variables,
+                   ArrayRef<Value *> PrivateVariables,
+                   ArrayRef<ReductionGenTy> ReductionGen,
+                   ArrayRef<AtomicReductionGenTy> AtomicReductionGen = {},
----------------
Introduce a struct for holding the info for each reduction? Would also make it easier to extend.
```
struct ReductionInfo {
  Value *Variable;
  Value *PrivateVariable;
  ReductionGenTy ReductionGen;
  AtomicReductionGenTy AtomicReductionGen;
}
```


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2749
+  GlobalVariable *LockVar =
+      M->getGlobalVariable(".gomp_critical_user_.reduction.var");
+  EXPECT_NE(LockVar, nullptr);
----------------
ftynse wrote:
> 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.
>From `Driver.h`:
```
    /// The GNU OpenMP runtime. Clang doesn't support generating OpenMP code for
    /// this runtime but can swallow the pragmas, and find and link against the
    /// runtime library itself.
    OMPRT_GOMP,
```

No idea why this name was chosen either, but seems we are stuck with it now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104928