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

[Daniel Berlin via llvm-commits]

dberlin added inline comments.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:646
@@ +645,3 @@
+        addToLeaderTable(Num, I, I->getParent());
+    }
+
----------------
weimingz wrote:
> dberlin wrote:
> > Close, but if you are adding only the new instructions, I believe this should be unconditional.
> > 
> > Think of the leader table as "the list of things in the program that have a given value number".
> > 
> > Even if the value number existed, if this is a new instruction, it's still a new thing that has that value number, and should still end up in the leader table.
> > 
> > (I'd also probably name it addNewInstruction or something)
> > 
> > 
> > 
> Is it possible that the new instruction has some existing GVN ? I was thinking calling processInstruction, but concerned that it may have side effect.
> 
It is possible the new instruction has an existing VN.

Imagine instruction is the lshr you have below, of a constant and a value.

Imagine we coerce a bunch of loads.

We are highly likely to insert the lshr a bunch of times, and it will be the same each time.
So t's entirely possible that lshr instruction already exists in the program, and vn_lookup_or_add will then find a value number for this instruction.

It happens that the current VN is not great, and so it won't find stuff unless they are pretty much completely identical.  processInstruction does a lot of forward propagation to try to make things look the same to account for this.

You also can't call processInstruction here because it may mess with or delete things or change the CFG or ...

(One of the other nice things new gvn does is split analysis and elimination, so that would never happen. It would be hard to fix this in existing GVN)

So i think what you are doing here is pretty much the best you can do in this situation, it should probably just be unconditional, as i mentioned.




http://reviews.llvm.org/D14479