[llvm] r272224 - [LoopSimplify] Preserve LCSSA when merging exit blocks.
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 28 23:52:25 PDT 2016
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>
>
More information about the llvm-commits
mailing list