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

[Weiming Zhao via llvm-commits]

weimingz added inline comments.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:646
@@ +645,3 @@
+        addToLeaderTable(Num, I, I->getParent());
+    }
+
----------------
dberlin wrote:
> 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.
> 
> 
Cool. Thanks for the detailed explanation.


http://reviews.llvm.org/D14479