[PATCH] D48851: [PM/LoopUnswitch] Fix the first and immediate components of PR37889.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D48851#1152398, @fedor.sergeev wrote:

> 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...)


Repository:
  rL LLVM

https://reviews.llvm.org/D48851