Page MenuHomePhabricator

[OMPIRBuilder] add minimalist reduction support
ClosedPublic

Authored by ftynse on Jun 25 2021, 9:56 AM.

Details

Summary

This introduces a builder function for emitting IR performing reductions in
OpenMP. Reduction variable privatization and initialization to the
reduction-neutral value is expected to be handled separately. The caller
provides the reduction functions. Further commits can provide implementation of
reduction functions for the reduction operators defined in the OpenMP
specification.

This implementation was tested on an MLIR fork targeting OpenMP from C and
produced correct executable code.

Diff Detail

Event Timeline

ftynse created this revision.Jun 25 2021, 9:56 AM
ftynse requested review of this revision.Jun 25 2021, 9:56 AM

Some cases that don't match LLVM style (method, function, guard). However, I think loop iterator cases (e, i, ...) are rather a problem with clang-tidy.

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? In the current form, would it be even legal to it multiple times with just one variable to reduce?

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
514

The reduction clause would need to be attached to a directive

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1105–1107

Remove commented-out code entirely?

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2522–2523

Consider using doxygen comments.

2749

Why gomp ("GNU OpenMP") in the name?

ftynse updated this revision to Diff 355802.Jul 1 2021, 2:15 AM
ftynse marked 4 inline comments as done.

Address review.

ftynse added a comment.Jul 1 2021, 2:26 AM

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.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2749

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.

ftynse updated this revision to Diff 355807.Jul 1 2021, 2:30 AM

Drop spurious file

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

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
572–575

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;
}
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2749

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.

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.

I was rather thinking of collecting the reductions and manifesting them in OpenMPIRBuilder::finalize like createParallel does. However, as you pointed out implementations likely know all the reductions, so there might not be useful. However, I like the idea of ReductionInfo.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
551–552

What are the requirement for the location? Such as: Has to be between the last local store of a reduced location and the end of the thread.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1038–1040

assert descriptions missing.

Seems like the caller has to decide whether to use atomics or not. I think the API should itself decide whether atomics are possible or not. That is, if all ReductionInfo::AtomicReductionGen have a value we can use atomics, otherwise we fall back to ReductionGen.

1045–1048

Loc.IP.getBlock() is used orften enough to store it in a variable.

1055–1057

This could be done before the previous Builder.SetInsertPoint without an InsertPointGuard

1083

Consider using getTypeStoreSize

1085–1087

[serious] This will try to reuse the same .omp.reduction.func for all reductions in the module. I think you will want to add a new function with private linkage.

1115

This produces a deprecated warning: Use the version that explicitly specifies the loaded type instead

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2625–2626

Could you give these a name just such they are easier to identifier in the dump?

2695–2710

Should OMPIRBuilder provide a set of standard reduction callbacks so users don't have to declare them?

2819

Use std::next (or case_begin() + 1). ++ is modifying its argument which suggests that that case_begin() returns a reference. I am somewhat surprised that this even works.

From cppreference.com:

Although the expression ++c.begin() often compiles, it is not guaranteed to do so: c.begin() is an rvalue expression, and there is no LegacyInputIterator requirement that specifies that increment of an rvalue is guaranteed to work. In particular, when iterators are implemented as pointers or its operator++ is lvalue-ref-qualified, ++c.begin() does not compile, while std::next(c.begin()) does.

ftynse updated this revision to Diff 362662.Jul 29 2021, 12:42 AM
ftynse marked 11 inline comments as done.

Address review

ftynse added inline comments.Jul 29 2021, 12:42 AM
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2625–2626

We can't set names on store instructions, can we?

2695–2710

Would be nice to have, but I'd prefer not to scope creep this patch.

ftynse updated this revision to Diff 362684.Jul 29 2021, 2:23 AM

Add a test with two reduction functions

Meinersbur accepted this revision.Jul 29 2021, 1:03 PM

LGTM. Just some requests for additional comments.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
517

Could you add a short explanation about the relation between ReductionGen and AtomicReductionGen, and what happens if the latter is null?

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2620–2621

Could you give these a name just such they are easier to identifier in the dump?

2625–2626

😉

2695–2710

Could you add a TODO then?

2993–2998

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.

This revision is now accepted and ready to land.Jul 29 2021, 1:03 PM
ftynse updated this revision to Diff 363009.Jul 30 2021, 2:47 AM
ftynse marked 3 inline comments as done.

Address more review

This revision was landed with ongoing or failed builds.Jul 30 2021, 4:58 AM
This revision was automatically updated to reflect the committed changes.