This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][OpenMPIRBuilder] Implement loop unrolling.
ClosedPublic

Authored by Meinersbur on Aug 9 2021, 6:22 AM.

Details

Summary

Add methods for loop unrolling to the OpenMPIRBuilder class and use them in Clang if -fopenmp-enable-irbuilder is enabled. The unrolling methods are:

  • unrollLoopFull
  • unrollLoopPartial
  • unrollLoopHeuristic

unrollLoopPartial and unrollLoopHeuristic can use compiler heuristics to automatically determine the unroll factor. If possible, that is if no CanonicalLoopInfo is required to pass to another method, metadata for LLVM's LoopUnrollPass is added. Otherwise the unroll factor is determined using the same heurstics as user by LoopUnrollPass. Not requiring a CanonicalLoopInfo, especially with unrollLoopHeuristic allows greater flexibility.

With full unrolling and partial unrolling with known unroll factor, instead of duplicating instructions by the OpenMPIRBuilder, the full unroll is still delegated to the LoopUnrollPass. In case of partial unrolling the loop is first tiled using the existing tileLoops methods, then the inner loop fully unrolled using the same mechanism.

Diff Detail

Event Timeline

Meinersbur created this revision.Aug 9 2021, 6:22 AM
Meinersbur requested review of this revision.Aug 9 2021, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2021, 6:22 AM
Herald added a subscriber: sstefan1. · View Herald Transcript
  • Use TargetMachine for unroll factor heuristic
jdoerfert accepted this revision.Aug 25 2021, 11:46 AM

LG, minor comments below.

clang/lib/CodeGen/CGStmtOpenMP.cpp
2636–2640

make it an unreachable or sink the assert and combine the conditions. Later allows to remove all braces.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2097

Nit: Add an assert for latch to exist. Just for my sanity ;)

2139

Should we cache it in the OpenMPIRBuilder?

2261
2295

This code matches the pattern we have in the 2 members above. Can we make it a member as well and make it clear in the name of all 3 that we use metadata and the unroller pass? No strong feelings, more of an idea.

This revision is now accepted and ready to land.Aug 25 2021, 11:46 AM

I have a few minor questions.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2075

Nit: In the body, it seems we are prepending.

2188–2189

Should this be settable for experimentation or additional control? If not can you provide an explanation for these values?

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1667

Are we not calling ompbuilder finalize or verify because it is only adding metadata?

1747

Should we add tests for workshare loops with unroll?

Meinersbur marked 5 inline comments as done.
  • Address reviews
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2075

In the body, first we append Existing->operands(), then Properties. I.e. we append the Properties passes as an argument to after the existing properties to form a new list NewLoopProperties.

2188–2189

Made them configurable using the new -openmp-ir-builder-unroll-threshold-factor switch.

I selected 1.5 as default to make it match approximately the unroll factor the LoopUnrollPass would derive in an -O3 pass pass pipeline. No large-scale experiments, just a loop that LoopUnrollPass would unroll by a factor of 4 (out of possible: 1 (no unrolling), 2, 4, 8) and make #pragma omp unroll partial also unroll by a factor of 4.

2295

I think the API should abstract over how the unrolling is performed, frontends should not need to be concerned about how unrolling is actually applied. The metadata version is preferred unless we cannot use it because we need the unrolled loop's CFG wrapped by CanonicalLoopInfo.

I changed the parameters to make it more idiomatic that the caller only receives a CanonicalLoopInfo if requested, and that's the only purpose of the 4th parameter.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1667

Added

OMPBuilder.finalize();
EXPECT_FALSE(verifyModule(*M, &errs()));

to the unroll tests. Thanks for noticing.

1747

I think this is beyond the scope of these tests here. To test thoroughly, we'd have to test all combinations of loop transformations and loop-associated directives. The CanonicalLoopInfo::assertOK() should already check the invariants that ensure that the CLI can be used as input for other loop-associated directives. Additionally, the composition is tested with clang's tests.

kiranchandramohan accepted this revision.Sep 1 2021, 8:24 AM

LGTM. Thanks for the responses.

This revision was landed with ongoing or failed builds.Sep 2 2021, 12:40 AM
This revision was automatically updated to reflect the committed changes.
lkail added a subscriber: lkail.EditedSep 2 2021, 12:55 AM

This looks have broken build if -DBUILD_SHARED_LIBS=On is specified.

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "LLVMFrontendOpenMP" of type SHARED_LIBRARY
    depends on "LLVMPasses" (weak)
  "LLVMipo" of type SHARED_LIBRARY
    depends on "LLVMFrontendOpenMP" (weak)
  "LLVMCoroutines" of type SHARED_LIBRARY
    depends on "LLVMipo" (weak)
  "LLVMPasses" of type SHARED_LIBRARY
    depends on "LLVMCoroutines" (weak)
    depends on "LLVMipo" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.
CMake Generate step failed.  Build files cannot be regenerated correctly.

Hi, this has broken the build for me too.

lebedev.ri reopened this revision.Sep 2 2021, 2:42 AM
This revision is now accepted and ready to land.Sep 2 2021, 2:42 AM
Meinersbur updated this revision to Diff 370464.EditedSep 2 2021, 6:34 PM

Register required passes individually instead of asking the PassBuilder to register them. The PassBuilder sits in the LLVMPasses component. This creates a circular dependency since LLVMPasses requires LLVMipo containing the OpenMPOpt pass that uses the OpenMPIRBuilder.

  • Also register indirect dependencies
This revision was landed with ongoing or failed builds.Sep 4 2021, 5:24 PM
This revision was automatically updated to reflect the committed changes.