[llvm] r176408 - recommit r172363 & r171325 (reverted in r172756)

[Shuxin Yang]

See following,

Also, I decide to make less attention to this issue. I cannot afford the 
time.
Not only triaging the problem exposed by your commit took me lots of 
time, it also took me
some time to let you pay attention to potential problem I figured out, 
and now I try to
  convince you breaking assumption would open a big can of worm, and you 
certainly
have alternative to avoid.

I would ask the list members to share insight on this issue. Since this 
original checkin
break bootstrap book with LTO, it may break the build in the future.

On 03/03/2013 09:30 AM, Nuno Lopes wrote:
>> I emphasize that your problematic should not resurrected without 
>> addressing my concerns I put
>> in PR14988. I'm rather disappointed that you still go ahead resurrect 
>> your change.
>>
>> The ObjectSizeOffsetVisitor::visitPhiNode() dosen't guarantee if all 
>> operands points to the same
>> objects. While it would not lead to problem if the function is called 
>> in the context of alias analysis.
>> It would cause problem when instcombine try to replace 
>> llvm.objectsize() to constant.
>
> I don't think that's true.
> It is correct that visitPhiNode() can return a known size if the phi 
> node may point to multiple objects.  But that will only happen if 
> *all* those objects have exactly the same size.
> When instcombine folds llvm.objectsize() to a constant, it doesn't 
> need to know *exactly* to which objects its argument is pointing to. 

How do you know all potential uses of this llvm.objectsize(p).  One 
might use this function to figure out if p is pointing to single object.
Your function break the assumption of ObjectSizeOffset class. While the 
class dose not explicitly claim the assumption is "single object",
the reader and the client of this class tend to take that for granted.

Can you guarantee that breaking such assumption is 100% safe for what we 
have today, and 100% for the future.



> But if all the pointed-to objects have the same size, then you can 
> still fold llvm.objectsize() to a constant, since you're sure that 
> whaetever object it will point to at run time, it will have the size 
> you computed.
>
> But please send an example if you believe I'm wrong.
>
I repeat over and over again, it is potential problem. Breaking an 
assumption tend to open a big can of worm.
Apparently, I believe you have alternative to solve this problem, by 
walking phi operand in getUnderlyingObject(),  to
rewind the pointer to the base address of underlying object, this way, 
you obviate the need to break the assumption
in ObjectSizeOffsetVisitor::visitPhiNode().

Also, in r176407, how about bring the comment to the "Object" back. I 
think that comment is useful for reader.




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130303/63b9340e/attachment.html>