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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
