This is an archive of the discontinued LLVM Phabricator instance.

[PM/LoopUnswitch] Fix the first and immediate components of PR37889.
ClosedPublic

Authored by chandlerc on Jul 2 2018, 2:40 PM.

Details

Summary

This PR illustrates that a fundamental analysis update was not performed
with the new loop unswitch. This update is also somewhat fundamental to
the core idea of the new loop unswitch -- we actually *update* the CFG
based on the unswitching. In order to do that, we need to update the
loop nest in addition to the domtree.

For some reason, when writing trivial unswitching, I thought that the
loop nest structure cannot be changed by the transformation. But the PR
helps illustrate that it clearly can. I've expanded this to a number of
different test cases that try to cover the different cases of this. When
we unswitch, we move an exit edge of a loop out of the loop. If this
exit edge changes which loop reached by an exit is the innermost loop,
it changes the parent of the loop. Essentially, this transformation may
hoist the inner loop up the nest.

I've added the simple logic to handle this reliably in the trivial
unswitching case. However, in the non-trivial case we have a harder
problem because we may be creating new sibling loops. I'm going to port
these test cases over to it to see if it too is missing some handling
for the hoisting, but the logic to implement it will likely need to be
a bit different, and similar to the logic we already use to hoist the
newly created sibling loops. Due to these differences, I'm sending this
out as a separate patch and then I'll send a follow-up that at least
adds test coverage and maybe adds the necessary logic, re-using what
I've added here if I see a good way to do that.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Jul 2 2018, 2:40 PM
chandlerc updated this revision to Diff 153849.Jul 2 2018, 7:20 PM

Add non-trivial test cases. The logic already correctly handled these, but this
gives me more confidence in having comprehensive testing.

Adding Fedor on the off chance he can review this sooner. Don't want this to languish too long as this is actually a latent bug in the wild -- not really sure why anything works w/o this...

Some small comments, but otherwise lgtm.

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
282 ↗(On Diff #153849)

To better test this loop, could we have a test with deeper nesting going upward more than one loop nest? e.g. A < B < C < D, where D becomes nested in A after unswitch?

llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch.ll
3374 ↗(On Diff #153849)

Comment?

llvm/test/Transforms/SimpleLoopUnswitch/trivial-unswitch.ll
971 ↗(On Diff #153849)

Add comment like for the other tests?

fedor.sergeev accepted this revision.Jul 3 2018, 2:05 PM

LGTM for the code.

I'm not sure whats your understanding of the state of this issue for non-trivial?
This code does not address the issue, but the tests supplied do not fail?
Do you need a test that fails before addressing non-trivial part?

This revision is now accepted and ready to land.Jul 3 2018, 2:05 PM

LGTM for the code.

I'm not sure whats your understanding of the state of this issue for non-trivial?
This code does not address the issue, but the tests supplied do not fail?

I think there is existing code that at least handles the cases in these tests. We had to handle similar stuff w/ non-trivial unswitching because we create new loops as well and we got lots of assert failures w/o handling those correctly. I mostly added the test cases here to demonstrate to myself (especially my future self) that at least these cases are already handled correctly.

Do you need a test that fails before addressing non-trivial part?

I mean, if there is a bug in the non-trivial part, yeah, we'll need to find it and the test cases that actually hit that path. So far, it looks OK.

Ok, makes sense.

Btw, I'm running downstream testing with this set of changes, but since I did not see anything like 37889 before I dont think its gonna find anything here as well.

Hmm... downstream testing found an issue.
Here is a reduced testcase (https://reviews.llvm.org/file/data/b4nvnqme23unyxwktst4/PHID-FILE-wny66mgcso2cbh4ciz3x/reduced-unswitch.ll)

Fails only with your changes applied on top of upstream:

] bin/opt -passes='require<opt-remark-emit>,loop(licm,unswitch)' reduced-unswitch.ll -disable-output
opt: /home/fsergeev/work/llvm-upstream/llvm/lib/Transforms/Scalar/LICM.cpp:350: bool {anonymous}::LoopInvariantCodeMotion::runOnLoop(llvm::Loop*, llvm::AliasAnalysis*, llvm::LoopInfo*, llvm::DominatorTree*, llvm::TargetLibraryInfo*, llvm::TargetTransformInfo*, llvm::ScalarEvolution*, llvm::MemorySSA*, llvm::OptimizationRemarkEmitter*, bool): Assertion `(!L->getParentLoop() || L->getParentLoop()->isLCSSAForm(*DT)) && "Parent loop not left in LCSSA form after LICM!"' failed.
...

Hmm... downstream testing found an issue.
Here is a reduced testcase (https://reviews.llvm.org/file/data/b4nvnqme23unyxwktst4/PHID-FILE-wny66mgcso2cbh4ciz3x/reduced-unswitch.ll)

Fails only with your changes applied on top of upstream:

] bin/opt -passes='require<opt-remark-emit>,loop(licm,unswitch)' reduced-unswitch.ll -disable-output
opt: /home/fsergeev/work/llvm-upstream/llvm/lib/Transforms/Scalar/LICM.cpp:350: bool {anonymous}::LoopInvariantCodeMotion::runOnLoop(llvm::Loop*, llvm::AliasAnalysis*, llvm::LoopInfo*, llvm::DominatorTree*, llvm::TargetLibraryInfo*, llvm::TargetTransformInfo*, llvm::ScalarEvolution*, llvm::MemorySSA*, llvm::OptimizationRemarkEmitter*, bool): Assertion `(!L->getParentLoop() || L->getParentLoop()->isLCSSAForm(*DT)) && "Parent loop not left in LCSSA form after LICM!"' failed.
...

Thanks for the test case.

The issue I missed is that we're creating new exits from all the no-longer-containing loops. We need to form LCSSA and dedicated exits there. I've adjusted my existing test cases to exercise LCSSA effectively and it covers both the case in your reduced testcase as well as several others (for example, intermediate loops). I need to polish these tests up, add the deeper nesting test case Alina requested, and port the enhancements over to the non-trivial side as well (where we already had to handle this for the same reasons as we had to handle the hoisting).

Once everything is looking good again, I'll probably just land. The fix is super obvious once i realized i needed to look.

(My kingdom for LCSSA to form PHI nodes in the preheader for use-by-nested-loop in addition to the exit, would make all of this much easier...)

uabelho added a subscriber: uabelho.Jul 6 2018, 1:59 AM
chandlerc marked 3 inline comments as done.Jul 6 2018, 5:57 PM
chandlerc updated this revision to Diff 154476.Jul 6 2018, 5:58 PM

Update with the bug found by Fedor fixed and test cases nicely enhanced to
cover all of this stuff.

I've also added some asserts to document that other possible updates to the
loop structure aren't needed. The code here is pretty trivial and the rest of
the patch already got LGTMs so I'm gonna go ahead and submit this.

This also updates to have the extra test requested in review and more comments
in the tests.

This revision was automatically updated to reflect the committed changes.