[PATCH] D107764: [OpenMP][OpenMPIRBuilder] Implement loop unrolling.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2055
+/// Attach loop metadata \p Properties to the loop described by \p Loop. If the
+/// loop already has metadata, the loop properties are appended.
+static void addLoopMetadata(CanonicalLoopInfo *Loop,
----------------
kiranchandramohan wrote:
> Nit: In the body, it seems we are prepending.
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`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2168-2169
+  // actually unrolls the loop.
+  UP.Threshold *= 1.5;
+  UP.PartialThreshold *= 1.5;
+
----------------
kiranchandramohan wrote:
> Should this be settable for experimentation or additional control? If not can you provide an explanation for these values?
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.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2275
+    return nullptr;
+  }
+
----------------
jdoerfert wrote:
> 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.
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.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1666
 
+TEST_F(OpenMPIRBuilderTest, UnrollLoopFull) {
+  OpenMPIRBuilder OMPBuilder(*M);
----------------
kiranchandramohan wrote:
> Are we not calling ompbuilder finalize or verify because it is only adding metadata?
Added
```
OMPBuilder.finalize();
EXPECT_FALSE(verifyModule(*M, &errs()));
```
to the unroll tests. Thanks for noticing.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1735
+
 TEST_F(OpenMPIRBuilderTest, StaticWorkShareLoop) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
----------------
kiranchandramohan wrote:
> Should we add tests for workshare loops with unroll?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107764