[llvm] r329047 - [SCEV] Make computeExitLimit more simple and more powerful
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 25 00:17:53 PDT 2018
Yeah, I'm working on getting the IR and the before/after diff of the IR due
to this patch. I have everything set up to compare.
Only one function changes too, gen_bitlen in trees.c, so it should be easy
to get a "good" and "bad" IR test case both starting from some unoptimized
bitcode.
On Wed, Apr 25, 2018 at 12:08 AM Maxim Kazantsev <max.kazantsev at azul.com>
wrote:
> Hi Chandler,
>
>
>
> Could you please send me the output of "clang -emit-llvm" before the
> optimizations are happening? That would help me to investigate it on my
> side.
>
>
>
> -- Max
>
>
>
> *From:* Chandler Carruth [mailto:chandlerc at gmail.com]
> *Sent:* Wednesday, April 25, 2018 1:56 PM
> *To:* Maxim Kazantsev <max.kazantsev at azul.com>
> *Cc:* Kit Barton <kbarton at ca.ibm.com>; Eric Christopher <
> echristo at google.com>; Han Shen <shenhan at google.com>; Hal Finkel <
> hfinkel at anl.gov>; dlj at google.com; llvm-commits at lists.llvm.org
>
>
> *Subject:* Re: [llvm] r329047 - [SCEV] Make computeExitLimit more simple
> and more powerful
>
>
>
> Passing -O1 just to the compile of trees.c from zlib results in the
> failure:
>
> https://github.com/madler/zlib/blob/master/trees.c
>
>
>
> Looking at getting a before/after diff by reverting just this patch.
>
>
>
> On Tue, Apr 24, 2018 at 11:49 PM Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
> Narrowing this down to a miscompile of zlib v1.2.11 which gets linked into
> Clang and which is used to compress and decompress the PCH file. This makes
> sense as the error is a corrupt zlib header.
>
>
>
> On Tue, Apr 24, 2018 at 11:21 PM Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
> Just FYI, Clang appears to continue to miscompile itself on PPC after
> incorporating all of these fixes. I'll try narrow down what file.
>
>
>
> On Mon, Apr 23, 2018 at 4:46 AM Maxim Kazantsev <max.kazantsev at azul.com>
> wrote:
>
> Fix is https://reviews.llvm.org/D45945
>
>
>
> *From:* Maxim Kazantsev
> *Sent:* Monday, April 23, 2018 5:02 PM
> *To:* 'Chandler Carruth' <chandlerc at gmail.com>
> *Cc:* Kit Barton <kbarton at ca.ibm.com>; Eric Christopher <
> echristo at google.com>; Han Shen <shenhan at google.com>; Hal Finkel <
> hfinkel at anl.gov>; dlj at google.com; llvm-commits at lists.llvm.org
>
>
> *Subject:* RE: [llvm] r329047 - [SCEV] Make computeExitLimit more simple
> and more powerful
>
>
>
> Here they have a reproducer on something that is very similar to that (and
> is not a duplicate): https://bugs.llvm.org/show_bug.cgi?id=37205
>
>
>
> I'm going to start looking at this.
>
>
>
> *From:* Chandler Carruth [mailto:chandlerc at gmail.com <chandlerc at gmail.com>]
>
>
> *Sent:* Monday, April 23, 2018 4:56 PM
> *To:* Maxim Kazantsev <max.kazantsev at azul.com>
>
> *Cc:* Chandler Carruth <chandlerc at gmail.com>; Kit Barton <
> kbarton at ca.ibm.com>; Eric Christopher <echristo at google.com>; Han Shen <
> shenhan at google.com>; Hal Finkel <hfinkel at anl.gov>; dlj at google.com;
> llvm-commits at lists.llvm.org
>
>
> *Subject:* Re: [llvm] r329047 - [SCEV] Make computeExitLimit more simple
> and more powerful
>
>
>
> FWIW, I think it is OK to land some of these without explicit testing
> given the clear evidence that they could be problematic. It should also be
> reasonable to add test cases reduced later from these failures if possible.
>
>
>
> But the easiest way for me to confirm whether these address the specific
> issue I've seen (but don't have a test case yet, nor know how to produce
> one easily) is to land the patches.
>
>
>
> On Mon, Apr 23, 2018 at 2:34 AM Maxim Kazantsev via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> One more place of potential problem is here:
> https://reviews.llvm.org/D45940
>
>
>
> Note that I don't have a test that would prove this leads to real
> miscompiles, and I'm not sure how to construct it, but common sense says
> that invalidating parent alone is not enough.
>
>
>
> *From:* Maxim Kazantsev
> *Sent:* Monday, April 23, 2018 1:05 PM
>
>
> *To:* 'Chandler Carruth' <chandlerc at gmail.com>; 'Kit Barton' <
> kbarton at ca.ibm.com>; 'Eric Christopher' <echristo at google.com>; 'Han Shen'
> <shenhan at google.com>; 'Hal Finkel' <hfinkel at anl.gov>; 'dlj at google.com' <
> dlj at google.com>
> *Cc:* 'llvm-commits at lists.llvm.org' <llvm-commits at lists.llvm.org>
>
> *Subject:* RE: [llvm] r329047 - [SCEV] Make computeExitLimit more simple
> and more powerful
>
>
>
> At least one of this variety is real. It seems that some passes *do*
> expect that only the innermost loop's cached data will be affected. I've
> submitted a fix for it: https://reviews.llvm.org/D45937
>
>
>
> I will keep looking for other issues of this kind. The symptom of the
> problem is that SE.verify() fails at some point (which means that its
> internal data is inconsistent).
>
>
>
> Hope it helps,
>
> Max
>
>
>
> *From:* Maxim Kazantsev
> *Sent:* Monday, April 23, 2018 9:32 AM
> *To:* 'Chandler Carruth' <chandlerc at gmail.com>; 'Kit Barton' <
> kbarton at ca.ibm.com>; 'Eric Christopher' <echristo at google.com>; 'Han Shen'
> <shenhan at google.com>; 'Hal Finkel' <hfinkel at anl.gov>; 'dlj at google.com' <
> dlj at google.com>
> *Cc:* 'llvm-commits at lists.llvm.org' <llvm-commits at lists.llvm.org>
> *Subject:* RE: [llvm] r329047 - [SCEV] Make computeExitLimit more simple
> and more powerful
>
>
>
> Actually I've started looking into the passes, and what happens in Loop
> Unswitching looks highly suspicious:
>
>
>
> if (auto *SEWP = getAnalysisIfAvailable<ScalarEvolutionWrapperPass>())
>
> SEWP->getSE().forgetLoop(L);
>
>
>
> We are doing this before we start modifying the loop, in particular
> inserting new blocks. It is pretty much like what was happening in
> unrolling. I haven't yet proved that it is a bug, but it is likely to be
> one. If you could disable loop unswitching and check if you still observe
> your failure without it, it would be very useful input.
>
>
>
> Thanks,
>
> Max
>
>
>
> *From:* Maxim Kazantsev
> *Sent:* Monday, April 23, 2018 8:26 AM
> *To:* 'Chandler Carruth' <chandlerc at gmail.com>; Kit Barton <
> kbarton at ca.ibm.com>; Eric Christopher <echristo at google.com>; Han Shen <
> shenhan at google.com>; Hal Finkel <hfinkel at anl.gov>; dlj at google.com
> *Cc:* llvm-commits at lists.llvm.org
> *Subject:* RE: [llvm] r329047 - [SCEV] Make computeExitLimit more simple
> and more powerful
>
>
>
> Hi Chandler,
>
>
>
> Thank you very much for getting me informed! I'd be happy to help to deal
> with this issue. Please provide exact steps for reproducing this issue once
> you track them down (I don't typically work with Clang and might need some
> links or instructions how to build the configuration you are mentioning).
>
>
>
> As for my suspicions (it is just a shot in the dark, I don't have solid
> arguments that this is exactly what is going on) is that some pass, likely
> close to backend, is misusing SCEV. We can end up with memory corruption if
> some optimization doesn't invoke "forgetLoop" for a loop it might have
> modified somehow. The old code could be just accidentally correct just
> because we could not compute anything for a loop which we failed to
> invalidate. In this case, it is something that must be fixed.
>
>
>
> Fortunately, there is not so many places where forgetLoop is used. I will
> now go through them and try to see what could be wrong in how it's done
> there. With the patch under suspicion, we calculate exit counts for more
> loops now.
>
>
>
> Please keep me informed on what you find!
>
>
>
> Best regards,
>
> Max
>
>
>
> *From:* Chandler Carruth [mailto:chandlerc at gmail.com <chandlerc at gmail.com>]
>
>
> *Sent:* Saturday, April 21, 2018 10:20 AM
> *To:* Maxim Kazantsev <max.kazantsev at azul.com>; Kit Barton <
> kbarton at ca.ibm.com>; Eric Christopher <echristo at google.com>; Han Shen <
> shenhan at google.com>; Hal Finkel <hfinkel at anl.gov>; dlj at google.com
>
> *Cc:* llvm-commits at lists.llvm.org
> *Subject:* Re: [llvm] r329047 - [SCEV] Make computeExitLimit more simple
> and more powerful
>
>
>
> This doesn't have anything to do with ASan FWIW, that just enables -O1.
>
>
>
> Essentially, we're seeing this test fail on a "normal" bootstrapped Clang
> with -O1 (or -O2) after this revision. But weirdly, the 3-stage PPC build
> bot isn't failing. Trying to figure out why next.
>
>
>
> On Fri, Apr 20, 2018 at 7:20 PM Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
> In case it wasn't clear, I have no reason to suspect the code in this
> commit is wrong. We've looked at it and the code looks totally fine.
>
>
>
> Much more likely that it is exposing some underlying bug somewhere much
> like the one fixed here: https://reviews.llvm.org/D44818
>
>
>
>
>
> On Fri, Apr 20, 2018 at 7:19 PM Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
> We're seeing a miscompile with this that is ... really frustrating to pin
> down.
>
>
>
> Specifically, it appears that if you build Clang itself using a Clang
> built after this revision (so a stage2 Clang binary in bootstrap parlance),
> and specifically build Clang itself with ASan enabled targeting PPC (no
> other architectures we test in this way appear to be impacted), and then
> use that asan+ppc stage2 Clang binary to run a specific OpenMP test
> (clang/test/OpenMP:for_simd_codegen.cpp), that test will fail. It will
> specifically fail the third RUN line with the output:
>
>
>
> error: 'error' diagnostics seen but not expected:
>
> (frontend): malformed or corrupted AST file: 'could not decompress
> embedded file contents: zlib error: Z_DATA_ERROR'
>
>
>
> It is possible we have a miscompiled zlib after this revision? Haven't yet
> pinned down whether the *write* of the PCH just above it wrote corrupt
> data, or the *read* failed to read perfectly correct data.
>
>
>
> Sadly, I don't have access to a PPC machine to easily reproduce this and
> test these things out. We just have an automated build that happens to use
> this configuration and spits out the failure. =/
>
>
>
> CC-ing various folks who may be able to help try to reproduce this
> usefully.
>
>
>
> Somewhat concerning is that this is the *only* failure we've seen testing
> past this revision. No other platform, no other test. Doesn't fail w/o
> ASan, etc etc. But it also is really reliable on our end.
>
>
>
> If anyone is trying to reproduce this, happy to chat with them on IRC and
> try to give the exact commands used at each stage of this.
>
>
>
> -Chandler
>
>
>
> PS: Huge props to +David Jones <dlj at google.com> for pinning down the
> cause of this.
>
>
>
> On Mon, Apr 2, 2018 at 11:00 PM Max Kazantsev via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: mkazantsev
> Date: Mon Apr 2 22:57:19 2018
> New Revision: 329047
>
> URL: http://llvm.org/viewvc/llvm-project?rev=329047&view=rev
> Log:
> [SCEV] Make computeExitLimit more simple and more powerful
>
> Current implementation of `computeExitLimit` has a big piece of code
> the only purpose of which is to prove that after the execution of this
> block the latch will be executed. What it currently checks is actually a
> subset of situations where the exiting block dominates latch.
>
> This patch replaces all these checks for simple particular cases with
> domination check over loop's latch which is the only necessary condition
> of taking the exiting block into consideration. This change allows to
> calculate exact loop taken count for simple loops like
>
> for (int i = 0; i < 100; i++) {
> if (cond) {...} else {...}
> if (i > 50) break;
> . . .
> }
>
> Differential Revision: https://reviews.llvm.org/D44677
> Reviewed By: efriedma
>
> Modified:
> llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> llvm/trunk/test/Analysis/ScalarEvolution/exact_iter_count.ll
>
> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=329047&r1=329046&r2=329047&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Mon Apr 2 22:57:19 2018
> @@ -6884,63 +6884,12 @@ ScalarEvolution::computeBackedgeTakenCou
> ScalarEvolution::ExitLimit
> ScalarEvolution::computeExitLimit(const Loop *L, BasicBlock *ExitingBlock,
> bool AllowPredicates) {
> - // Okay, we've chosen an exiting block. See what condition causes us
> to exit
> - // at this block and remember the exit block and whether all other
> targets
> - // lead to the loop header.
> - bool MustExecuteLoopHeader = true;
> - BasicBlock *Exit = nullptr;
> - for (auto *SBB : successors(ExitingBlock))
> - if (!L->contains(SBB)) {
> - if (Exit) // Multiple exit successors.
> - return getCouldNotCompute();
> - Exit = SBB;
> - } else if (SBB != L->getHeader()) {
> - MustExecuteLoopHeader = false;
> - }
> -
> - // At this point, we know we have a conditional branch that determines
> whether
> - // the loop is exited. However, we don't know if the branch is
> executed each
> - // time through the loop. If not, then the execution count of the
> branch will
> - // not be equal to the trip count of the loop.
> - //
> - // Currently we check for this by checking to see if the Exit branch
> goes to
> - // the loop header. If so, we know it will always execute the same
> number of
> - // times as the loop. We also handle the case where the exit block
> *is* the
> - // loop header. This is common for un-rotated loops.
> - //
> - // If both of those tests fail, walk up the unique predecessor chain to
> the
> - // header, stopping if there is an edge that doesn't exit the loop. If
> the
> - // header is reached, the execution count of the branch will be equal
> to the
> - // trip count of the loop.
> - //
> - // More extensive analysis could be done to handle more cases here.
> - //
> - if (!MustExecuteLoopHeader && ExitingBlock != L->getHeader()) {
> - // The simple checks failed, try climbing the unique predecessor chain
> - // up to the header.
> - bool Ok = false;
> - for (BasicBlock *BB = ExitingBlock; BB; ) {
> - BasicBlock *Pred = BB->getUniquePredecessor();
> - if (!Pred)
> - return getCouldNotCompute();
> - TerminatorInst *PredTerm = Pred->getTerminator();
> - for (const BasicBlock *PredSucc : PredTerm->successors()) {
> - if (PredSucc == BB)
> - continue;
> - // If the predecessor has a successor that isn't BB and isn't
> - // outside the loop, assume the worst.
> - if (L->contains(PredSucc))
> - return getCouldNotCompute();
> - }
> - if (Pred == L->getHeader()) {
> - Ok = true;
> - break;
> - }
> - BB = Pred;
> - }
> - if (!Ok)
> - return getCouldNotCompute();
> - }
> + assert(L->contains(ExitingBlock) && "Exit count for non-loop block?");
> + // If our exiting block does not dominate the latch, then its
> connection with
> + // loop's exit limit may be far from trivial.
> + const BasicBlock *Latch = L->getLoopLatch();
> + if (!Latch || !DT.dominates(ExitingBlock, Latch))
> + return getCouldNotCompute();
>
> bool IsOnlyExit = (L->getExitingBlock() != nullptr);
> TerminatorInst *Term = ExitingBlock->getTerminator();
> @@ -6955,9 +6904,19 @@ ScalarEvolution::computeExitLimit(const
> /*ControlsExit=*/IsOnlyExit, AllowPredicates);
> }
>
> - if (SwitchInst *SI = dyn_cast<SwitchInst>(Term))
> + if (SwitchInst *SI = dyn_cast<SwitchInst>(Term)) {
> + // For switch, make sure that there is a single exit from the loop.
> + BasicBlock *Exit = nullptr;
> + for (auto *SBB : successors(ExitingBlock))
> + if (!L->contains(SBB)) {
> + if (Exit) // Multiple exit successors.
> + return getCouldNotCompute();
> + Exit = SBB;
> + }
> + assert(Exit && "Exiting block must have at least one exit");
> return computeExitLimitFromSingleExitSwitch(L, SI, Exit,
>
> /*ControlsExit=*/IsOnlyExit);
> + }
>
> return getCouldNotCompute();
> }
>
> Modified: llvm/trunk/test/Analysis/ScalarEvolution/exact_iter_count.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/exact_iter_count.ll?rev=329047&r1=329046&r2=329047&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Analysis/ScalarEvolution/exact_iter_count.ll (original)
> +++ llvm/trunk/test/Analysis/ScalarEvolution/exact_iter_count.ll Mon Apr
> 2 22:57:19 2018
> @@ -25,3 +25,37 @@ exit:
> side.exit:
> ret void
> }
> +
> +define void @test_02(i1 %c) {
> +
> +; CHECK-LABEL: Determining loop execution counts for: @test_02
> +; CHECK-NEXT: Loop %loop: <multiple exits> backedge-taken count is 50
> +
> +entry:
> + br label %loop
> +
> +loop:
> + %iv = phi i32 [ 0, %entry ], [ %iv.next, %backedge ]
> + br i1 %c, label %if.true, label %if.false
> +
> +if.true:
> + br label %merge
> +
> +if.false:
> + br label %merge
> +
> +merge:
> + %side.cond = icmp slt i32 %iv, 50
> + br i1 %side.cond, label %backedge, label %side.exit
> +
> +backedge:
> + %iv.next = add i32 %iv, 1
> + %loop.cond = icmp slt i32 %iv, 100
> + br i1 %loop.cond, label %loop, label %exit
> +
> +exit:
> + ret void
> +
> +side.exit:
> + ret void
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://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/20180425/2485beb9/attachment.html>
More information about the llvm-commits
mailing list