Jump Theading/GVN bug

[Philip Reames]

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] *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 3^rd 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/118fe0ba/attachment.html>