Jump Theading/GVN bug

[Hal Finkel]

Hi Katya,

My understanding of our general policy is that, if a pass cannot handle invalid unreachable code, it is responsible for avoiding it itself. Consider, for example the SLP vectorizer, which contains this:

  if (!DT->isReachableFromEntry(BB)) {
    // Don't go into unreachable blocks. They may contain instructions with
    // dependency cycles which confuse the final scheduling.
    DEBUG(dbgs() << "SLP: bundle in unreachable block.\n");
    newTreeEntry(VL, false);
    return;
  }

I suspect that GVN should also ignore unreachable code in a similar way.

 -Hal

----- Original Message -----
> From: "Katya Romanova" <Katya_Romanova at playstation.sony.com>
> To: llvm-commits at cs.uiuc.edu
> Sent: Sunday, February 22, 2015 3:34:58 AM
> 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.
> 
> 
> 
> 
> 
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory