[llvm] r272224 - [LoopSimplify] Preserve LCSSA when merging exit blocks.
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 5 06:15:08 PDT 2016
hi,
I added a TR for this;
https://llvm.org/bugs/show_bug.cgi?id=28424
Regards,
Mikael
On 07/04/2016 10:41 AM, Mikael Holmén wrote:
> Hi Michael,
>
> This weekend our csmith tests caused LoopSimplify to crash again. This
> time it's the checks before "simplifyLoop" that fails so I suppose this
> is actually a bug in LCSSA?.
>
> If I do
>
> opt -S -lcssa -indvars red5.ll
>
> on the attached file I get
>
> opt: ../lib/Transforms/Utils/LoopSimplify.cpp:792: virtual bool
> (anonymous namespace)::LoopSimplify::runOnFunction(llvm::Function &):
> Assertion `InLCSSA && "Requested to preserve LCSSA, but it's already
> broken."' failed.
>
> Regards,
> Mikael
>
> On 06/29/2016 08:52 AM, Mikael Holmén via llvm-commits wrote:
>> Hi Michael,
>>
>> On 06/28/2016 06:04 PM, Michael Zolotukhin wrote:
>>>
>>>> On Jun 28, 2016, at 3:30 AM, Mikael Holmén <mikael.holmen at ericsson.com
>>>> <mailto:mikael.holmen at ericsson.com>> wrote:
>>>>
>>>> Hi Michael,
>>>>
>>>> (I've been on vacation for a few days, hence my delayed answer)
>>>>
>>>> On 06/23/2016 03:38 PM, Michael Zolotukhin wrote:
>>>>>
>>>>>> On Jun 23, 2016, at 2:09 AM, Mikael Holmén
>>>>>> <mikael.holmen at ericsson.com <mailto:mikael.holmen at ericsson.com>>
>>>>>> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 06/23/2016 01:54 AM, Michael Zolotukhin wrote:
>>>>>>>
>>>>>>>> On Jun 22, 2016, at 1:25 AM, Mikael Holmén
>>>>>>>> <mikael.holmen at ericsson.com <mailto:mikael.holmen at ericsson.com>>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hi Michael,
>>>>>>>>
>>>>>>>> The checks introduced in this commit fail on:
>>>>>>>>
>>>>>>>> opt -S -lcssa -loop-unroll red.ll
>>>>>>>>
>>>>>>>> I don't know if it's the checks or the optimization that is
>>>>>>>> actually wrong.
>>>>>>> Hi Mikael,
>>>>>>>
>>>>>>> The checks are correct, they just exposed a bug that has been there
>>>>>>> for a long time. It seems that a routine separateNestedLoop in
>>>>>>> LoopSimplify breaks LCSSA - the test you provided shows exactly
>>>>>>> that. I’ll look into how we can fix it.
>>>>>>
>>>>>> Great!
>>>>>>
>>>>>>> Does it block you in any way?
>>>>>>
>>>>>> Well we see the checks failing quite alot in our testing now, so I
>>>>>> simply turned off the preservation of LCSSA in LoopSimplify as a
>>>>>> workaround that I hope is ok until the bug is fixed for real.
>>>>>>
>>>>>> // Temporarily turn off preservation of LCSSA to prevent #10729 from
>>>>>> // triggering all the time.
>>>>>> #if 0
>>>>>> AU.addPreservedID(LCSSAID);
>>>>>> #endif
>>>>>>
>>>>>> I suppose this should be ok?
>>>>> I would suggest a different fix:
>>>>>
>>>>> diff --git a/lib/Transforms/Utils/LoopSimplify.cpp
>>>>> b/lib/Transforms/Utils/LoopSimplify.cpp
>>>>> index 2509b0a..90dd621 100644
>>>>> --- a/lib/Transforms/Utils/LoopSimplify.cpp
>>>>> +++ b/lib/Transforms/Utils/LoopSimplify.cpp
>>>>> @@ -338,6 +338,7 @@ static Loop *separateNestedLoop(Loop *L,
>>>>> BasicBlock *Preheader,
>>>>> --i;
>>>>> }
>>>>> }
>>>>> + llvm::formLCSSARecursively(*L, *DT, LI, SE);
>>>>>
>>>>> return NewOuter;
>>>>> }
>>>>>
>>>>> Could you please try that
>>>>
>>>> Sure!
>>>>
>>>> The specific code that I attached earlier in this thread seems to work
>>>> with this fix instead of my workaround.
>>>>
>>>> I've run a bunch of our own normal tests and I haven't seen any
>>>> problems so far with your change so that seems good as well.
>>>>
>>>> However, the basic test
>>>>
>>>> test/Transforms/LoopSimplify/phi-node-simplify.ll
>>>>
>>>> seems to fail with it?
>>>>
>>>> No idea if this is important but I get:
>>>>
>>>> Expected 6 lines, got 10
>>>>
>>>> with your change.
>>>>
>>>> So, if you just do something about phi-node-simplify.ll I suppose all
>>>> is well.
>>> Hi Mikael,
>>>
>>> Yes, you’re right, it’ll break phi-node-simplify.ll. To unbreak it, you
>>> could guard the call to llvm::formLCSSARecursively with “if
>>> (PreserveLCSSA)”. However, for now I consider this just as a workaround,
>>> I hope I’ll come up with a smarter fix soon.
>>
>> Great! With
>>
>> if (PreserveLCSSA)
>> llvm::formLCSSARecursively(*L, *DT, LI, SE);
>>
>> I haven't seen any issues so I'll use this as a workaround from now on.
>>
>> Thanks,
>> Mikael
>>
>>>
>>> Thanks,
>>> Michael
>>>
>>>
>>>>
>>>> Thanks,
>>>> Mikael
>>>>
>>>>> As far as I understand, it should cover all cases where LCSSA is
>>>>> broken and restore it (and if not it’ll give us a valuable info about
>>>>> other cases :-) ).
>>>>>
>>>>> In your version of the fix there are a couple of drawbacks, which
>>>>> actually can hurt you:
>>>>> 1) When a pass says that it doesn’t preserve LCSSA, LoopPassManager
>>>>> can’t schedule it along with other loop passes if they do preserve
>>>>> LCSSA. In result, you get two separate pass managers, which means
>>>>> your run all passes from the first PM for all loops first, then all
>>>>> pass managers from the second PM on all loops. The problem is that
>>>>> some passes from the first PM might rely on LoopSimplify, which is
>>>>> now scheduled in the second PM.
>>>>> 2) Most of the loop passes assume that loops are in LCSSA form. But
>>>>> if a pass breaks it for one loop, no one would recompute it before
>>>>> proceeding to another loop: it only can be recomputed when all loops
>>>>> are processed by this pass manager. In this case I think it won’t
>>>>> lead to a bug because AFAIR LoopSimplify doesn’t rely on LCSSA and
>>>>> it’ll be scheduled in its one PM without other passes, but still,
>>>>> it’s a slippery road.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Mikael
>>>>>>
>>>>>>>
>>>>>>> Michael
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Mikael
>>>>>>>>
>>>>>>>> On 06/09/2016 01:13 AM, Michael Zolotukhin via llvm-commits wrote:
>>>>>>>>> Author: mzolotukhin
>>>>>>>>> Date: Wed Jun 8 18:13:21 2016
>>>>>>>>> New Revision: 272224
>>>>>>>>>
>>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=272224&view=rev
>>>>>>>>> Log:
>>>>>>>>> [LoopSimplify] Preserve LCSSA when merging exit blocks.
>>>>>>>>>
>>>>>>>>> Summary:
>>>>>>>>> This fixes PR26682. Also add LCSSA as a preserved pass to
>>>>>>>>> LoopSimplify,
>>>>>>>>> that looks correct to me and allows to write a test for the issue.
>>>>>>>>>
>>>>>>>>> Reviewers: chandlerc, bogner, sanjoy
>>>>>>>>>
>>>>>>>>> Subscribers: llvm-commits
>>>>>>>>>
>>>>>>>>> Differential Revision: http://reviews.llvm.org/D21112
>>>>>>>>>
>>>>>>>>> Added:
>>>>>>>>> llvm/trunk/test/Transforms/LoopSimplify/pr26682.ll
>>>>>>>>> Modified:
>>>>>>>>> llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp
>>>>>>>>>
>>>>>>>>> Modified: llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp
>>>>>>>>> URL:
>>>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp?rev=272224&r1=272223&r2=272224&view=diff
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ==============================================================================
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --- llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp (original)
>>>>>>>>> +++ llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp Wed Jun 8
>>>>>>>>> 18:13:21 2016
>>>>>>>>> @@ -686,8 +686,10 @@ ReprocessLoop:
>>>>>>>>> }
>>>>>>>>> DT->eraseNode(ExitingBlock);
>>>>>>>>>
>>>>>>>>> - BI->getSuccessor(0)->removePredecessor(ExitingBlock);
>>>>>>>>> - BI->getSuccessor(1)->removePredecessor(ExitingBlock);
>>>>>>>>> + BI->getSuccessor(0)->removePredecessor(
>>>>>>>>> + ExitingBlock, /* DontDeleteUselessPHIs */
>>>>>>>>> PreserveLCSSA);
>>>>>>>>> + BI->getSuccessor(1)->removePredecessor(
>>>>>>>>> + ExitingBlock, /* DontDeleteUselessPHIs */
>>>>>>>>> PreserveLCSSA);
>>>>>>>>> ExitingBlock->eraseFromParent();
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>> @@ -748,6 +750,7 @@ namespace {
>>>>>>>>> AU.addPreserved<GlobalsAAWrapperPass>();
>>>>>>>>> AU.addPreserved<ScalarEvolutionWrapperPass>();
>>>>>>>>> AU.addPreserved<SCEVAAWrapperPass>();
>>>>>>>>> + AU.addPreservedID(LCSSAID);
>>>>>>>>> AU.addPreserved<DependenceAnalysisWrapperPass>();
>>>>>>>>> AU.addPreservedID(BreakCriticalEdgesID); // No critical
>>>>>>>>> edges added.
>>>>>>>>> }
>>>>>>>>> @@ -781,11 +784,27 @@ bool LoopSimplify::runOnFunction(Functio
>>>>>>>>> SE = SEWP ? &SEWP->getSE() : nullptr;
>>>>>>>>> AC =
>>>>>>>>> &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
>>>>>>>>> bool PreserveLCSSA = mustPreserveAnalysisID(LCSSAID);
>>>>>>>>> +#ifndef NDEBUG
>>>>>>>>> + if (PreserveLCSSA) {
>>>>>>>>> + assert(DT && "DT not available.");
>>>>>>>>> + assert(LI && "LI not available.");
>>>>>>>>> + bool InLCSSA =
>>>>>>>>> + all_of(*LI, [&](Loop *L) { return
>>>>>>>>> L->isRecursivelyLCSSAForm(*DT); });
>>>>>>>>> + assert(InLCSSA && "Requested to preserve LCSSA, but it's
>>>>>>>>> already broken.");
>>>>>>>>> + }
>>>>>>>>> +#endif
>>>>>>>>>
>>>>>>>>> // Simplify each loop nest in the function.
>>>>>>>>> for (LoopInfo::iterator I = LI->begin(), E = LI->end(); I !=
>>>>>>>>> E; ++I)
>>>>>>>>> Changed |= simplifyLoop(*I, DT, LI, SE, AC, PreserveLCSSA);
>>>>>>>>>
>>>>>>>>> +#ifndef NDEBUG
>>>>>>>>> + if (PreserveLCSSA) {
>>>>>>>>> + bool InLCSSA =
>>>>>>>>> + all_of(*LI, [&](Loop *L) { return
>>>>>>>>> L->isRecursivelyLCSSAForm(*DT); });
>>>>>>>>> + assert(InLCSSA && "LCSSA is broken after loop-simplify.");
>>>>>>>>> + }
>>>>>>>>> +#endif
>>>>>>>>> return Changed;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Added: llvm/trunk/test/Transforms/LoopSimplify/pr26682.ll
>>>>>>>>> URL:
>>>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopSimplify/pr26682.ll?rev=272224&view=auto
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ==============================================================================
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --- llvm/trunk/test/Transforms/LoopSimplify/pr26682.ll (added)
>>>>>>>>> +++ llvm/trunk/test/Transforms/LoopSimplify/pr26682.ll Wed Jun 8
>>>>>>>>> 18:13:21 2016
>>>>>>>>> @@ -0,0 +1,32 @@
>>>>>>>>> +; RUN: opt < %s -lcssa -loop-simplify -indvars -S | FileCheck %s
>>>>>>>>> +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
>>>>>>>>> +target triple = "x86_64-unknown-unknown"
>>>>>>>>> +
>>>>>>>>> + at a = external global i32, align 4
>>>>>>>>> +
>>>>>>>>> +; Check that loop-simplify merges two loop exits, but preserves
>>>>>>>>> LCSSA form.
>>>>>>>>> +; CHECK-LABEL: @foo
>>>>>>>>> +; CHECK: for:
>>>>>>>>> +; CHECK: %or.cond = and i1 %cmp1, %cmp2
>>>>>>>>> +; CHECK-NOT: for.cond:
>>>>>>>>> +; CHECK: for.end:
>>>>>>>>> +; CHECK: %a.lcssa = phi i32 [ %a, %for ]
>>>>>>>>> +define i32 @foo(i32 %x) {
>>>>>>>>> +entry:
>>>>>>>>> + br label %for
>>>>>>>>> +
>>>>>>>>> +for:
>>>>>>>>> + %iv = phi i32 [ 0, %entry ], [ %iv.next, %for.cond ]
>>>>>>>>> + %cmp1 = icmp eq i32 %x, 0
>>>>>>>>> + %iv.next = add nuw nsw i32 %iv, 1
>>>>>>>>> + %a = load i32, i32* @a
>>>>>>>>> + br i1 %cmp1, label %for.cond, label %for.end
>>>>>>>>> +
>>>>>>>>> +for.cond:
>>>>>>>>> + %cmp2 = icmp slt i32 %iv.next, 4
>>>>>>>>> + br i1 %cmp2, label %for, label %for.end
>>>>>>>>> +
>>>>>>>>> +for.end:
>>>>>>>>> + %a.lcssa = phi i32 [ %a, %for ], [ %a, %for.cond ]
>>>>>>>>> + ret i32 %a.lcssa
>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> llvm-commits mailing list
>>>>>>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>>>>
>>>>>>>> <red.ll>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list