[PATCH] D14670: Fix bug 25440: GVN assertion after coercing loads

[Daniel Berlin via llvm-commits]

This is actually exactly what i feared.
"So it seems we do need to check if a VN exists before adding to LaderTable
in addNewInstruction().
"
No, it means the leader table is messed up.

THe check you are trying to add is equivalent to this:


02382   // If the number we were assigned was a brand new VN, then we
don't02383   // need to do a lookup to see if the number already
exists02384   // somewhere in the domtree: it can't!02385   if (Num >=
NextNum) {02386     addToLeaderTable(Num, I, I->getParent
<http://llvm.org/docs/doxygen/html/classllvm_1_1Instruction.html#a9cd49851904f15060edb782ef4dd1b2d>());02387
    return false;02388   }


As i mentioned, it's doing this because it's not sensible to call
findLeader on a new value number.

Replicating this in the new instruction code fixes nothing. It's just plain
wrong ;-)
In fact, your code does this:

        unsigned Num = VN.lookup_or_add(I);
        uint32_t NextNum = VN.getNextUnusedValueNumber();

This is the wrong order to call these in ;-)
You want to get the next unused number before lookup/add it.

In any case,  the bug you have found is that findLeader(VN for ptrtoint,
BB) is returning the wrong answer, as I suspect.

It should return %sub.ptr.rhs.cast25 in that BB, but it's returning %0.

This is because the leader table management is a bit broken, and expects to
only have a single value per bb (which makes little to no sense if you
handle loads, but GVN doesn't so it can get away with it).

One possible fix i can think of:

Right now you have:

 // Assign VNs for instructions newly created during GVN optimization.
    void addNewInstruction(Value *Val) {
      if (Instruction *I = dyn_cast<Instruction>(Val)) {
        unsigned Num = VN.lookup_or_add(I);
        uint32_t NextNum = VN.getNextUnusedValueNumber();
        if (Num >= NextNum)
          addToLeaderTable(Num, I, I->getParent());
      }
    }

change it to
 // Assign VNs for instructions newly created during GVN optimization.
    Value *findOrAddNewInstruction(Value *Val) {
      if (Instruction *I = dyn_cast<Instruction>(Val)) {
// Note: I reversed the order to be correct
        uint32_t NextNum = VN.getNextUnusedValueNumber();
        unsigned Num = VN.lookup_or_add(I);
        if (Num >= NextNum) {
          addToLeaderTable(Num, I, I->getParent());
          return nullptr;
        } else if (Value *Existing = findLeader(I->getParent(), Num)) {
          return Existing;
        } else {
          addToLeaderTable(Num, I, I->getParent());
       }
      }
    }


Call add new instruction, and if it returns non null, replace the
instruction you created with the return value (or GVN will do it later for
you if you like).

If you'd rather have GVN do that part, change it to:
    void addNewInstruction(Value *Val) {
      if (Instruction *I = dyn_cast<Instruction>(Val)) {
       // Note: I reversed the order to be correct
        uint32_t NextNum = VN.getNextUnusedValueNumber();
        unsigned Num = VN.lookup_or_add(I);
        if (Num >= NextNum) {
          addToLeaderTable(Num, I, I->getParent());
        } else if (findLeader(I->getParent(), Num) != nullptr) {
          return;
        } else {
          addToLeaderTable(Num, I, I->getParent());
       }
      }
    }

GVN will do the propagation for you, at the expense of extra findLeader
calls



On Tue, Nov 17, 2015 at 10:43 AM, Weiming Zhao <weimingz at codeaurora.org>
wrote:

> weimingz added a comment.
>
> Hi Berlin,
>
> Here is what’s happening:
>
> When it sees the load in BB if.then.14:
>
>   %v1 = load i32, i32* bitcast (i8** @dfg_text to i32*), align 4
>
> Then, it tries to do PRE.
> So it finds the store in BB while.end:
>
>   store i8* %add.ptr, i8** @dfg_text, align 4
>
> So, in GetStoreValueForLoad, it inserts a PtrToInt (line 1153)
>
> Now, the BB becomes
>
> while.end:                                        ; preds = %while.cond.16
>
>   %add.ptr = getelementptr inbo  %sub.ptr.rhs.cast25 unds i8, i8* %v2, i32
> undef
>   store i8* %add.ptr, i8** @dfg_text, align 4
>   %sub.ptr.rhs.cast25 = ptrtoint i8* %add.ptr to i32  *** original
> ptrtoint ***
>   %sub.ptr.sub26 = sub i32 0, %sub.ptr.rhs.cast25
>   %0 = ptrtoint i8* %add.ptr to i32   *** newly created ***
>   switch i32 undef, label %sw.default [
>
> This new instruction happens to have the same GVN as %sub.ptr.rhs.cast25
>
> So, when GVN process BB while.end, if finds that %sub.ptr.rhs.cast25
> already have a GVN, so deleted it and replaces the use with %0, which is
> wrong.
>
> So it seems we do need to check if a VN exists before adding to LaderTable
> in addNewInstruction().
>
>
> http://reviews.llvm.org/D14670
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151117/30f8b637/attachment.html>