[llvm] r353911 - [LoopSimplifyCFG] Re-enable const branch folding by default
Alina Sbirlea via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 21 19:07:13 PST 2019
Great, thank you for following up!
On Thu, Feb 21, 2019, 6:49 PM Roman Tereshin <rtereshin at apple.com> wrote:
> Hi Alina,
>
> It all works now as far as I can tell.
>
> Thank you!
>
> Roman
>
> On Feb 21, 2019, at 11:41 AM, Alina Sbirlea <alina.sbirlea at gmail.com>
> wrote:
>
> A couple of general notes:
>
> 1. Please avoid using splitBasicBlock() per the comment:
> /// "Also note that this doesn't preserve any passes. To split blocks while
> /// keeping loop information consistent, use the SplitBlock utility
> function."
>
> Doing this forces manual updates to DT and LI, that are already handled by
> the utility and in this case also changed the default naming scheme for
> split blocks.
>
> 2. The DT applyUpdates implementation is such that if the given updates
> don't match what the DT has or are too complex, it will give up and
> recompute from scratch.
> With MemorySSA we're trying very hard to not do that, because it can get
> expensive. So the list of updates tolerated by the DT is larger. With
> MemorySSA it needs to be "just right".
>
> 3. Please don't add -debug-only flags in unit tests unless that output is
> checked. It makes for a very noisy output.
>
>
> Patch: https://reviews.llvm.org/D58524 should fix the issue.
> Roman, please let me know if you the unreduced case still sees issues
> after this patch. I'm inclined to accept post-commit feedback and land the
> fix for now.
>
>
> Thanks,
> Alina
>
>
>
> On Thu, Feb 21, 2019 at 9:12 AM Alina Sbirlea <alina.sbirlea at gmail.com>
> wrote:
>
>> FWIW, there are also a few bugs open that I have on my list and one of
>> them may overlap with this failure.
>>
>> To answer your question, Roman, no, not planning to turn on
>> enable-mssa-loop-dependency just yet, especially with open bugs.
>> Unfortunately, since the coverage in the unit tests is fairly low, changes
>> to the passes that used to preserve MemorySSA are likely to introduce new
>> bugs if they are not tested with the option enabled.
>> Current plan is to solve the bugs that came in this week and get more
>> coverage in the unit tests.
>>
>> Best,
>> Alina
>>
>> On Thu, Feb 21, 2019 at 7:36 AM Alina Sbirlea <alina.sbirlea at gmail.com>
>> wrote:
>>
>>> 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/661f9c3b/attachment-0001.html>
More information about the llvm-commits
mailing list