[PATCH] Remove seemingly dead InstCombine compare code

Matt Arsenault Matthew.Arsenault at amd.com
Mon Aug 5 16:27:47 PDT 2013


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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130805/d2b3adc4/attachment.html>


More information about the llvm-commits mailing list