Jump Theading/GVN bug
Daniel Berlin
dberlin at dberlin.org
Tue Mar 24 16:57:48 PDT 2015
+1.
I actually don't think we'll come to any sane conclusion on the unreachable
code front anytime soon, the most likely conclusion is we keep on trucking
and fix what breaks until it becomes too hard to do so, then reevaluate
with real data since we last tried :)
If that means some passes need to do a quick cleanup pre-pass to avoid
massive messes in their algorithms, that sounds like the right thing to me.
If most passes really are expecting/dealing with unreachable code, that
really shouldn't be too bad.
If it becomes too expensive or whatever, we reevaluate whether it's the
right set of tradeoffs.
Whether current GVN is one of those algorithms where it's better to do a
pre-pass cleanup, i can't say. It actually seems pretty easy to just not
walk into blocks that have no preds when generating the RPO order in
iterateOnFunction.
I don't see the disadvantage.
The new algorithm i have already skips unreachable blocks anyway because it
does unreachable block detecting, so it's handling them.
On Tue, Mar 24, 2015 at 4:32 PM, Philip Reames <listmail at philipreames.com>
wrote:
> Personally, I would prefer to see the short term fix implemented as his
> "(2) GVN should skip unreachable code;".
>
> Given there's already a pre-pass merging blocks into their predecessor,
> simply deleting blocks which aren't reachable (according to the dom tree)
> seems quite straight forward and inexpensive.
>
> Philip
>
>
> On 03/19/2015 05:10 PM, Gao, Yunzhong wrote:
>
> Hi,
>
> Sorry to re-open an old thread here. We have a compiler based on LLVM 3.6
> branch, and the compiler hangs
>
> when compiling one of our games due to this bug. I am aware that there is
> on-going discussion in llvmdev
>
> regarding the long-term solution of the general design of LLVM passes,
>
>
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150223/261463.html
>
> , which looks like it will take a while before any consensus is reached. I
> wonder if Katya’s patch could serve as a
>
> stop-gap solution for the short term.
>
> What do you think?
>
> - Gao
>
>
>
>
>
> *From:* llvm-commits-bounces at cs.uiuc.edu [
> mailto:llvm-commits-bounces at cs.uiuc.edu <llvm-commits-bounces at cs.uiuc.edu>]
> *On Behalf Of *Romanova, Katya
> *Sent:* Sunday, February 22, 2015 1:35 AM
> *To:* llvm-commits at cs.uiuc.edu
> *Subject:* Jump Theading/GVN bug
>
>
>
> Hello,
>
>
>
> I encountered a problem triggered by Jump-Threading optimization. This
> pass is creating an unreachable block with an instruction that is not well
> formed, which then causes the subsequent GVN pass to enter an infinite loop.
>
>
>
> Here are the details:
>
> Consider the following small testcase. I reduced it from a huge project
> that was failing during LTO. Unfortunately, I was not given any source
> files, only the bitcode.
>
>
>
>
> -----------------------------------------------------------------------------
>
> small.ll
>
> ----------------------------------------------------------------
>
> %"class.ls_file::Path" = type { [1024 x i8], i8*, i8*, i8*}
>
>
>
> ; Function Attrs: nounwind sspstrong uwtable
>
> define void @foo(%"class.ls_file::Path"* %this, i8* nocapture readonly
> %pPath) #0 {
>
> %1 = load i8* %pPath, align 1
>
> switch i8 %1, label %.loopexit.i.i [
>
> i8 92, label %2
>
> i8 47, label %2
>
> ]
>
>
>
> ; <label>:2 ; preds = %0, %0
>
> %3 = load i8* %pPath, align 1
>
> %4 = icmp eq i8 %3, 46
>
> br i1 %4, label %.critedge.i.i, label %.outer.i.i
>
>
>
> .critedge.i.i: ; preds =
> %.critedge.i.i, %.critedge.i.i, %2
>
> %.0.i.i = phi i8* [ %pPath, %2 ], [ %5, %.critedge.i.i ], [ %5,
> %.critedge.i.i ]
>
> %5 = getelementptr inbounds i8* %.0.i.i, i64 1
>
> %6 = load i8* %5, align 1
>
> switch i8 %6, label %.outer.i.i [
>
> i8 92, label %.critedge.i.i
>
> i8 47, label %.critedge.i.i
>
> ]
>
>
>
> .outer.i.i: ; preds =
> %.critedge.i.i, %2
>
> %.2.ph2.i.i = phi i8* [ %5, %.critedge.i.i ], [ %pPath, %2 ]
>
> %7 = load i8* %.2.ph2.i.i, align 1
>
> %8 = icmp eq i8 %7, 0
>
> br label %.loopexit.i.i
>
>
>
> .loopexit.i.i: ; preds = %7
>
> ret void
>
> }
>
>
>
>
>
> If you run GVN after jump threading, opt will go into infinite recursion.
>
>
>
> opt -jump-threading small.ll -o test2.o
>
> opt -gvn test2.o -o test3.o
>
> Segmentation fault (core dumped)
>
>
>
>
>
> Here’s why it’s happening:
>
>
>
> Jump threading transforms this block:
>
>
>
> .critedge.i.i: ; preds =
> %.critedge.i.i, %.critedge.i.i, %2
>
> %.0.i.i = phi i8* [ %pPath, %2 ], [ %5, %.critedge.i.i ], [ %5,
> %.critedge.i.i ]
>
> %5 = getelementptr inbounds i8* %.0.i.i, i64 1
>
> %6 = load i8* %5, align 1
>
> switch i8 %6, label %.outer.i.i [
>
> i8 92, label %.critedge.i.i
>
> i8 47, label %.critedge.i.i
>
> ]
>
>
>
> into the following block:
>
>
>
> .critedge.i.i: ; preds =
> %.critedge.i.i, %.critedge.i.i
>
> %2 = getelementptr inbounds i8* %2, i64 1
>
> %3 = load i8* %2, align 1
>
> switch i8 %3, label %.outer.i.i [
>
> i8 92, label %.critedge.i.i
>
> i8 47, label %.critedge.i.i
>
> ]
>
>
>
> Note that the block .critedge.i.i is now unreachable and it contains the
> following instruction:
>
> %2 = getelementptr inbounds i8* %2, i64 1
>
>
>
> This instruction doesn’t appear to be well formed. However, one could
> argue that it is – formally %2 def does dominate all %2 uses because both
> are located in an unreachable block, so every path to the use is passing
> through the def (because there are 0 paths like that).
>
> That is likely the reason why –verify doesn’t complain about it.
>
>
>
>
>
> And this is where the problem actually happens:
>
> When -gvn pass is invoked afterwards, it goes into an infinite recursion
> in function ‘PHITranslateSubExpr’ (located in PHITransAddr.cpp). When
> processing a GEP instruction (%2), this function calls itself recursively
> with its first operand (which is also %2).
>
>
>
> // Handle getelementptr with at least one PHI translatable operand.
>
> if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Inst)) {
>
> SmallVector<Value*, 8> GEPOps;
>
> bool AnyChanged = false;
>
> for (unsigned i = 0, e = GEP->getNumOperands(); i != e; ++i) {
>
> Value *GEPOp = PHITranslateSubExpr(GEP->getOperand(i), CurBB,
> PredBB, DT); // <- it goes into infinite recursion here.
>
> if (GEPOp == 0) return 0;
>
>
>
> There are several different ways to fix this problem.
>
>
>
> (1) We have to remove the unreachable code
> immediately after or during Jump-Threading pass;
>
> (2) GVN should skip unreachable code;
>
> (3) GVN shouldn’t recursively invoke the same
> function with the same arguments.
>
>
>
>
>
> Here is an extra check that implements the 3rd approach. Please review.
>
>
>
> ===================================================================
>
> --- PHITransAddr.cpp (revision 229821)
>
> +++ PHITransAddr.cpp (working copy)
>
> @@ -217,10 +217,13 @@
>
> SmallVector<Value*, 8> GEPOps;
>
> bool AnyChanged = false;
>
> for (unsigned i = 0, e = GEP->getNumOperands(); i != e; ++i) {
>
> - Value *GEPOp = PHITranslateSubExpr(GEP->getOperand(i), CurBB,
> PredBB, DT);
>
> + Value *Operand= GEP->getOperand(i);
>
> + if (GEP==Operand)
>
> + continue;
>
> + Value *GEPOp = PHITranslateSubExpr(Operand, CurBB, PredBB, DT);
>
> if (!GEPOp) return nullptr;
>
>
>
> - AnyChanged |= GEPOp != GEP->getOperand(i);
>
> + AnyChanged |= GEPOp != Operand;
>
> GEPOps.push_back(GEPOp);
>
> }
>
>
>
> ===================================================================
>
> One thing to note here is – unreachable code, if not removed, could push
> invalid code through subsequent passes. This invalid code will not be
> caught by the verifier. If these passes are not paranoid enough, the bad
> code may cause all sorts of issues. The case above is one example.
>
>
>
>
>
> Katya.
>
>
>
>
>
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150324/0e1c91fe/attachment.html>
More information about the llvm-commits
mailing list