[PATCH] Remove seemingly dead InstCombine compare code

[Eli Friedman]

On Mon, Aug 5, 2013 at 4:27 PM, Matt Arsenault
<Matthew.Arsenault at amd.com> wrote:
> On 08/02/2013 04:47 PM, Eli Friedman wrote:
>
> On Thu, Aug 1, 2013 at 1:34 PM, Matt Arsenault
> <Matthew.Arsenault at amd.com> wrote:
>
> Turn check / insert cast into assertions that the sizes are the same. Add
> some more tests to try to break it. These paths seemed to not be tested with
> a datalayout. This checks if it needs to cast GEP indices, but at this point
> they should have already been converted to the pointer sized integer.
>
>
> http://llvm-reviews.chandlerc.com/D1261
>
> Files:
>   lib/Transforms/InstCombine/InstCombineCompares.cpp
>   test/Transforms/InstCombine/getelementptr.ll
>   test/Transforms/InstCombine/icmp.ll
>   test/Transforms/InstCombine/load-cmp.ll
>
> I'm pretty sure this patch is wrong.  instcombine will always
> eventually transform GEPs in the way you expect, and in trivial cases,
> we will visit the GEP before the icmp, but I think we can visit the
> icmp first in more complicated cases.
>
> -Eli
>
> I've looked at the order InstCombine tries to do things again, and I'm more
> confident now that this situation can't happen. The initially created
> worklists reversed so the pre-existing instructions are visited in order, so
> the simple case is always fine. A cmp using a loaded value from the result
> of a GEP will necessarily be visited after the GEP is visited and
> transformed.

That's true... but things can get nastier when PHI nodes are involved.
 I don't think we actually perform any transformations which actually
break this rule, but you're depending on a property of our PHI node
optimizations which isn't fundamental.

> The order gets a bit confusing when an instruction is replaced. The users of
> the instructions are added to the worklist after the result, so they will be
> visited before the replaced instruction and other initial instructions not
> yet visited. This would be a problem if the cmp instruction were a user of a
> replaced instruction. The cmps in question must have a constant operand, and
> then the pointer from the GEP. Therefore, it can't be a user of some other
> instruction that could be replaced that would cause it to be visited
> earlier. Items are also moved to the top of the worklist only if they aren't
> already in the map of seen instructions, so any instructions that existed
> before the InstCombine iteration began wouldn't be impacted and should be
> visited in the right order. This would be a problem if InstCombine
> introduced a new GEP with the wrong size indices that was used by a cmp, but
> it shouldn't be doing that.

I'm pretty sure we do in fact do this; see, for example,
InstCombiner::visitAllocaInst.

-Eli