This is an archive of the discontinued LLVM Phabricator instance.

Fix full loop unrolling initialization in new pass manager
ClosedPublic

Authored by echristo on Dec 18 2019, 8:14 PM.

Details

Summary
Fix full unrolling with new pass manager.

Last we looked at this and couldn't come up with a reason to change
it, but with a pragma for full loop unrolling we bypass every other
loop unroll and then fail to fully unroll a loop when the pragma is set.

Move the OnlyWhenForced out of the check and into the initialization
of the full unroll pass in the new pass manager. This doesn't show up
with the old pass manager.

Add a new option to opt so that we can turn off loop unrolling
manually since this is a difference between clang and opt.

Tested with check-clang and check-llvm.

Diff Detail

Event Timeline

echristo created this revision.Dec 18 2019, 8:14 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 18 2019, 8:14 PM
echristo updated this revision to Diff 234656.Dec 18 2019, 8:16 PM

Formatting and parens changes.

Unit tests: pass. 61011 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61011 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Harbormaster completed remote builds in B42772: Diff 234656.

Ping ping goes the trolley.

MaskRay added inline comments.
clang/test/Misc/loop-opt-setup.c
13

The test seems to pass without the code change (llvm/lib/Passes/PassBuilder.cpp) below.

mehdi_amini accepted this revision.Dec 26 2019, 3:36 PM
This revision is now accepted and ready to land.Dec 26 2019, 3:36 PM
mehdi_amini added inline comments.Dec 26 2019, 3:37 PM
llvm/lib/Passes/PassBuilder.cpp
505

I would document that the normal unroll pass does not honor the forced unroll, it is surprising to me.

Can we add an LLVM test w/ the metadata so that we have an entirely LLVM test flow that ensures the pass builder DTRT?

(I still would include the Clang side test which is also very useful to test integrating Clang w/ different flows through the pass manager.)

echristo updated this revision to Diff 250229.Mar 13 2020, 9:13 AM

Fixed the clang test.

Tried to get something that I could reduce down and duplicate with just opt but it's been... difficult. Even the small clang testcase in isolation won't duplicate via something like:

clang -O0 -fexperimental-new-pass-manager foo.cc -S -o - -emit-llvm -mllvm -disable-llvm-optzns | opt -passes='default<O1>' -transform-warning -pass-remarks-missed=transform-warning -S -o -

Might be holding it wrong, but this isn't very discoverable if so :)

echristo updated this revision to Diff 250232.Mar 13 2020, 9:17 AM

Fix comments around full unroller.

echristo marked an inline comment as done.Mar 13 2020, 9:17 AM
vitalybuka accepted this revision.Mar 25 2020, 1:17 PM
vitalybuka added a subscriber: vitalybuka.

Can this be landed?

echristo updated this revision to Diff 261928.May 4 2020, 2:06 PM
echristo edited the summary of this revision. (Show Details)

Add a testcase with opt and command line option so we can enable it.

echristo marked an inline comment as done.May 4 2020, 3:49 PM

Wooot about finally having a test case! (Sorry for nit picking it a bit ....)

llvm/test/Transforms/LoopUnroll/FullUnroll.ll
4–6 ↗(On Diff #261928)

Make sure it is in the correct function at least, and maybe after the label for the loop header?

12–17 ↗(On Diff #261928)

Are both functions needed?

19–20 ↗(On Diff #261928)

Nit, but minimize and clean this function up a touch?

At the least, removing all the target features seems valuable, and I'd give things stable names instead of numbered values.

echristo updated this revision to Diff 262458.May 6 2020, 12:55 PM

Update and reduce testcase a bit.

echristo marked 6 inline comments as done.May 6 2020, 12:57 PM

OK, ready again :)

llvm/test/Transforms/LoopUnroll/FullUnroll.ll
4–6 ↗(On Diff #261928)

Got it. There's not a lot of function left at the end:

define void @walrus() local_unnamed_addr #0 {
entry:

br label %for.cond.preheader

for.cond.preheader: ; preds = %for.cond.preheader, %entry

br label %for.cond.preheader

}

12–17 ↗(On Diff #261928)

I thought so, but apparently not :)

19–20 ↗(On Diff #261928)

Got it. Most things have stable names, only a few temporaries have numbered names.

chandlerc added inline comments.May 12 2020, 11:56 PM
clang/test/Misc/loop-opt-setup.c
12

This is dead now that you have different prefixes...

32–33

But for which function?

I'd rework these to be more modern CHECK-LABEL and affirmative checks for the unrolling.

llvm/test/Transforms/LoopUnroll/FullUnroll.ll
4–6 ↗(On Diff #261928)

Then switch to generated CHECK lines?

The checks we have here will pass easily after incorrect edits that cause this test to not actually check what it intends to. I'd either check something that constructively shows unrolling or generate exact checks (if its small enough to be genuinely stable).

19–20 ↗(On Diff #261928)

Sure, but even those will make future edits to this really frustrating with irrelevant diff, etc.

And this test case can still be minimized. As an example, the function is currently attributed as *optnone*. =/

I think you can just run this through the metarenamer pass?

echristo marked 7 inline comments as done.May 29 2020, 7:16 PM

Done. New diff incoming.

clang/test/Misc/loop-opt-setup.c
12

So it is. I'd actually missed this comment and saw it when fixing up other things. Thanks :)

echristo updated this revision to Diff 267424.May 29 2020, 7:17 PM
echristo marked an inline comment as done.

Update and restructure some test cases.

chandlerc accepted this revision.May 29 2020, 8:03 PM

Cool, thanks and LGTM!

echristo closed this revision.Jun 25 2020, 3:40 PM

Committed a while back.

I'm looking at enabling the -enable-npm-optnone flag and FullUnroll.ll fails. I understand that loop unrolling should be forced when some metadata is present, but the FullUnroll.ll test seems to check for a lot more than that. It checks for (roughly) two unconditional branches and no conditional branches. Running the test under -debug-pass-manager and --print-after-all, , it looks like InstCombine does a lot of work removing instructions, and SimplifyCFG is ultimately what removes all the conditional branches. But InstCombine and SimplifyCFG shouldn't run on an optnone function, right?