[llvm] r353911 - [LoopSimplifyCFG] Re-enable const branch folding by default

Thu Feb 21 07:36:57 PST 2019

Taking a look today.

On Thu, Feb 21, 2019, 3:55 AM Maxim Kazantsev <max.kazantsev at azul.com>
wrote:

> Hi again,
>
>
>
> Actually I’m in a little harsh about that. Tomorrow is my last day at
> Azul, and then I’ll be not available for a week or so due to a move. J
> I’ve added my personal email to be able to continue this discussion after
> that.
>
>
>
> I tried using MSSA->applyUpdates to solve this (the comment claims that
> it’s the exactly the way how it should work). For some reason, I am hitting
> the same assert on almost all tests where I try it. Here is the patch I
> tried (with some extra debug messages): https://reviews.llvm.org/D58500
>
>
>
> The failure on test test/Transforms/LoopSimplifyCFG/lcssa.ll happens in
> the very first function when we do updates at all (handleDeadExits). We
> accumulate updates corresponding to edges we add/remove. Then, we apply
> these updates to DTUpdater, and DT is able to pass verification after that
> (that means the updates are correct). But when we then try to apply the
> same set up updates to MSSAU, it fails with the assertion:
>
>
>
> opt:
> /home/mkazantsev/work/llvm/llvm/lib/Analysis/MemorySSAUpdater.cpp:848: void
> llvm::MemorySSAUpdater::applyInsertUpdates(llvm::ArrayRef<llvm::cfg::Update<llvm::BasicBlock*>
> >, llvm::DominatorTree&, const llvm::GraphDiff<llvm::BasicBlock*, false>*):
> Assertion `DT.dominates(NewIDom, PrevIDom) && "New idom should dominate old
> idom"' failed.
>
>
>
> I am not familiar with the ways how MSSA works at all, and I don’t
> understand why this assert is there and if it is possible to live without
> it. I also wasn’t able to find a method that would do full “recalculation”
> as a temporary fix for that. L
>
>
>
> I’ve committed the test that Roman has provided as XFAIL. Alina, could you
> please take a look into my debug patch above? Maybe you’ll have some
> insights what is missing there. Is there a way to fully recalculate MSSAU
> until we find a way to make it efficiently?
>
>
>
> If there is no good solution for that, I can only propose to turn off
> terminator folding when we need to update MSSAU (which would be a bad thing
> to do).
>
>
>
> --Max
>
>
>
> *From:* rtereshin at apple.com <rtereshin at apple.com>
> *Sent:* Thursday, February 21, 2019 3:51 PM
> *To:* Maxim Kazantsev <max.kazantsev at azul.com>; Alina Sbirlea <
> alina.sbirlea at gmail.com>
> *Cc:* dberlin at dberlin.org; llvm-commits <llvm-commits at lists.llvm.org>;
> asbirlea at google.com
> *Subject:* Re: [llvm] r353911 - [LoopSimplifyCFG] Re-enable const branch
> folding by default
>
>
>
> Yes, I saw her reply too (Hi Alina!). No rush, unless MemorySSA is forced
> on for loop passes (-enable-mssa-loop-dependency=true) it doesn’t break
> anything I’m aware of.
>
> I noticed the problem because I’m working on a change to LoopRotate, which
> is also supposed to preserve MemorySSA, so while testing that part I hit a
> few failures not related to my changes.
>
> I can just ignore them for time being or force
> -enable-loop-simplifycfg-term-folding off for testing.
>
>
>
> Also, I’m not going to be available for at least 10 hours (time zones).
>
>
>
> Thank you,
>
> Roman
>
>
>
> P.S. I’m also having difficulties with updating MemorySSA properly. Looks
> like MemorySSAUpdater contains rather per-case high level APIs, which can
> for instance handle a BB being split or merged into its necessarily
> unique predecessor.
>
> What I’m doing is more like SplitBlockAndInsertIfThen, which creates a new
> BB with 2 predecessors. MemorySSAUpdater::applyUpdates crashes
> with assert(AddedBlockSet.size() == 1 && "Can only handle adding one
> predecessor to a new block.”)
>
> if I try updating all edges in one call, or seemingly finishes fine if I
> do them one by one, but then MemorySSA verifier complains on an incomplete
> MemoryPhi, which makes me think applyUpdates didn’t really do the right
> thing anyway.
>
>
>
> What surprised me the most is that MemorySSA::createMemoryPhi is a private
> member function. After some running in circles I think I found a way to
> update MemorySSA properly, but I had to make MemorySSA::createMemoryPhi
>
> and MemorySSA::getWritableBlockDefs public for that for a time being
> (locally).
>
>
>
> Not sure if I’m missing something, or there is a valid problem with APIs
> here. Maybe *Alina* could give us some insight about that?
>
>
>
> Thanks,
>
> Roman
>
>
>
> On Feb 21, 2019, at 12:23 AM, Maxim Kazantsev <max.kazantsev at azul.com>
> wrote:
>
>
>
> Hi Roman,
>
>
>
> As Alina has pointed out, we do MSSA update wrong in general in this
> patch. I’m going to prepare a fix in ~3 hours from now. I’ll send it to you
> for review when it’s ready.
>
>
>
> --Max
>
>
>
> *From:* rtereshin at apple.com <rtereshin at apple.com>
> *Sent:* Thursday, February 21, 2019 2:41 PM
> *To:* Maxim Kazantsev <max.kazantsev at azul.com>
> *Cc:* Alina Sbirlea <alina.sbirlea at gmail.com>; dberlin at dberlin.org;
> llvm-commits <llvm-commits at lists.llvm.org>; asbirlea at google.com
> *Subject:* Re: [llvm] r353911 - [LoopSimplifyCFG] Re-enable const branch
> folding by default
>
>
>
> Hi Max,
>
>
>
> Thank you for replying so quickly!
>
>
>
> I’ve applied the change locally and the original IR module I used to
> bugpoint the reproducer I’ve sent you earlier failed anyway, this time for
> a different reason.
>
> I can’t send the module as it is for IP reasons, but here’s another
> bugpoint from the same source (with some manual tidying up):
>
>
>
> define void @main(i32* %a, i32* %b) {
>
> entry:
>
>   br label %for.body
>
>
>
> for.body:
>
>   %i = phi i32 [ 0, %entry ], [ %i.inc, %latch ]
>
>   br label %switch.bb
>
>
>
> switch.bb:
>
>   switch i2 1, label %default [
>
>     i2 1, label %case
>
>   ]
>
>
>
> case:
>
>   br label %latch
>
>
>
> default:
>
>   unreachable
>
>
>
> latch:
>
>   store i32 %i, i32* %a
>
>   store i32 %i, i32* %b
>
>   %i.inc = add nsw i32 %i, 1
>
>   %exitcond = icmp eq i32 %i.inc, 4
>
>   br i1 %exitcond, label %exit, label %for.body
>
>
>
> exit:
>
>   ret void
>
> }
>
>
>
> ./bin/opt -loop-simplifycfg
> loop-simplifycfg-term-folding-AND-mssa-loop-dependency-2.ll
> -enable-mssa-loop-dependency=true -verify-memoryssa=true
> -enable-loop-simplifycfg-term-folding=true -S -o -
>
> Assertion failed: (dominates(MD, U) && "Memory Def does not dominate it's
> uses"), function verifyDomination, file ../lib/Analysis/MemorySSA.cpp, line
> 1912.
>
>
>
> If I don’t apply https://reviews.llvm.org/rL354547 it fails the same way
> the previous reproducer did, so I guess this test is strictly better. Sorry
> for overbugpointing the first time around!
>
>
>
> Thanks,
>
> Roman
>
>
>
>
>
>
> On Feb 20, 2019, at 10:12 PM, Alina Sbirlea <alina.sbirlea at gmail.com>
> wrote:
>
>
>
> Are you sure your update is complete?
> It should theoretically match the DT update, i.e. include insert edges not
> just the delete. All updates should be done together using the same list of
> updates used for DT, and an up to date DT (see
> MSSAU->applyUpdates(DTUpdates, DT)).
>
>
>
> On Wed, Feb 20, 2019 at 9:53 PM Maxim Kazantsev via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Should be fixed by https://reviews.llvm.org/rL354547
>
>
>
> *From:* Maxim Kazantsev
> *Sent:* Thursday, February 21, 2019 11:49 AM
> *To:* 'rtereshin at apple.com' <rtereshin at apple.com>
> *Cc:* llvm-commits <llvm-commits at lists.llvm.org>; dberlin at dberlin.org;
> asbirlea at google.com
> *Subject:* RE: [llvm] r353911 - [LoopSimplifyCFG] Re-enable const branch
> folding by default
>
>
>
> Hi Roman,
>
>
>
> Thanks for notification! I’ll fix it asap.
>
>
>
> --Max
>
>
>
> *From:* rtereshin at apple.com <rtereshin at apple.com>
> *Sent:* Thursday, February 21, 2019 10:25 AM
> *To:* Maxim Kazantsev <max.kazantsev at azul.com>
> *Cc:* llvm-commits <llvm-commits at lists.llvm.org>; dberlin at dberlin.org;
> asbirlea at google.com
> *Subject:* Re: [llvm] r353911 - [LoopSimplifyCFG] Re-enable const branch
> folding by default
>
>
>
> Hi Max,
>
> Looks like this doesn’t collaborate with MemorySSA very well:
>
> ./bin/opt -loop-simplifycfg
> loop-simplifycfg-term-folding-AND-mssa-loop-dependency.ll
> -enable-mssa-loop-dependency=true -verify-memoryssa=true
> -enable-loop-simplifycfg-term-folding=true -S -o -
> Assertion failed: (find(predecessors(&B), Phi->getIncomingBlock(I)) !=
> pred_end(&B) && "Incoming phi block not a block predecessor"), function
> verifyDefUses, file ../lib/Analysis/MemorySSA.cpp, line 1947.
>
> The IR is:
>
> define void @main() {
> entry:
>   br label %for.body
>
> for.body:                                         ; preds = %exit, %entry
>   br label %switch.bb
>
> switch.bb:                                        ; preds = %for.body
>   switch i2 1, label %default.bb [
>     i2 1, label %case.bb
>   ]
>
> case.bb:                                          ; preds = %switch
>   br label %exit
>
> default.bb:                                       ; preds = %switch
>   unreachable
>
> exit:                                             ; preds = %case.bb
>   call void @foo()
>   br label %for.body
> }
>
> declare void @foo()
>
>
> Thanks,
> Roman
>
>
>
> > On Feb 12, 2019, at 10:12 PM, Max Kazantsev via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> > Author: mkazantsev
> > Date: Tue Feb 12 22:12:48 2019
> > New Revision: 353911
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=353911&view=rev
> > Log:
> > [LoopSimplifyCFG] Re-enable const branch folding by default
> >
> > Known underlying bugs have been fixed, intensive fuzz testing did not
> > find any new problems. Re-enabling by default. Feel free to revert if
> > it causes any functional failures.
> >
> > Modified:
> >    llvm/trunk/lib/Transforms/Scalar/LoopSimplifyCFG.cpp
> >
> > Modified: llvm/trunk/lib/Transforms/Scalar/LoopSimplifyCFG.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopSimplifyCFG.cpp?rev=353911&r1=353910&r2=353911&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Transforms/Scalar/LoopSimplifyCFG.cpp (original)
> > +++ llvm/trunk/lib/Transforms/Scalar/LoopSimplifyCFG.cpp Tue Feb 12
> 22:12:48 2019
> > @@ -41,7 +41,7 @@ using namespace llvm;
> > #define DEBUG_TYPE "loop-simplifycfg"
> >
> > static cl::opt<bool>
> EnableTermFolding("enable-loop-simplifycfg-term-folding",
> > -                                       cl::init(false));
> > +                                       cl::init(true));
> >
> > STATISTIC(NumTerminatorsFolded,
> >           "Number of terminators folded to unconditional branches");
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190221/4ed3d3c7/attachment.html>