[llvm] r272224 - [LoopSimplify] Preserve LCSSA when merging exit blocks.
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 23 06:38:00 PDT 2016
> 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? 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>
>>
>
More information about the llvm-commits
mailing list