[llvm] r272224 - [LoopSimplify] Preserve LCSSA when merging exit blocks.
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 4 01:41:41 PDT 2016
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
-------------- next part --------------
; ModuleID = 'red4.ll'
source_filename = "bugpoint-output-7eb0c3b.bc"
target triple = "x86_64-unknown-linux-gnu"
%rec950.2.389.392.395.398.401.431.434.437.443.2.1.1 = type { i32, i32, %rec948.1.388.391.394.397.400.430.433.436.442.1.0.0 }
%rec948.1.388.391.394.397.400.430.433.436.442.1.0.0 = type { i32, i32, i32, i32, i64, i32, i64 }
@g_494 = external global %rec950.2.389.392.395.398.401.431.434.437.443.2.1.1
@g_1589 = external global %rec948.1.388.391.394.397.400.430.433.436.442.1.0.0
@g_1609 = external global %rec948.1.388.391.394.397.400.430.433.436.442.1.0.0
declare i32 @safe_div_func_int32_t_s_s(i32, i32)
declare void @foo()
define internal i32 @func_36(i64 %p_37.404.par) {
br label %bb1
bb1: ; preds = %bb1, %0
br i1 undef, label %bb1, label %bb6
bb6: ; preds = %bb67, %bb1
%_tmp10402 = trunc i64 %p_37.404.par to i32
%_tmp10403 = call i32 @safe_div_func_int32_t_s_s(i32 undef, i32 %_tmp10402)
br i1 undef, label %bb76, label %bb66
bb67: ; preds = %bb66
br i1 undef, label %bb6, label %bb66
bb66: ; preds = %bb6, %bb67
%_tmp12391 = load i32, i32* getelementptr inbounds (%rec948.1.388.391.394.397.400.430.433.436.442.1.0.0, %rec948.1.388.391.394.397.400.430.433.436.442.1.0.0* @g_1609, i64 0, i32 5), align 4
%_tmp12392 = icmp slt i32 %_tmp12391, -29
br i1 %_tmp12392, label %bb67, label %bb70
bb70: ; preds = %bb66
store i32 1, i32* getelementptr inbounds (%rec950.2.389.392.395.398.401.431.434.437.443.2.1.1, %rec950.2.389.392.395.398.401.431.434.437.443.2.1.1* @g_494, i64 0, i32 1), align 4
br label %bb85
bb76: ; preds = %bb6
%_tmp12767 = load i64, i64* getelementptr inbounds (%rec948.1.388.391.394.397.400.430.433.436.442.1.0.0, %rec948.1.388.391.394.397.400.430.433.436.442.1.0.0* @g_1589, i64 0, i32 6), align 4
%_tmp12768 = icmp eq i64 %_tmp12767, 0
br i1 %_tmp12768, label %bb85, label %bb77
bb77: ; preds = %bb76
call void @foo()
unreachable
bb85: ; preds = %bb85, %bb76, %bb70
br i1 undef, label %bb85, label %bb130
bb130: ; preds = %bb85
ret i32 %_tmp10402
}
More information about the llvm-commits
mailing list