[llvm] r272224 - [LoopSimplify] Preserve LCSSA when merging exit blocks.
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 28 09:04:52 PDT 2016
> On Jun 28, 2016, at 3:30 AM, Mikael Holmén <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> 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> 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.
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
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>
>>>>> <red.ll>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160628/4a316a07/attachment.html>
More information about the llvm-commits
mailing list