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

[Arnold Schwaighofer]

On Mar 3, 2013, at 12:39 PM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:

> 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.
> 
> 


I have to agree with Shuxin here. The LangRef says about the semantics of "llvm.objectsize.i32/i64":

"The llvm.objectsize intrinsic is designed to provide information to the optimizers to determine at compile time whether a) an operation (like memcpy) will overflow a buffer that corresponds to an object, or b) that a runtime check for overflow isn’t necessary. An object in this context means an allocation of a specific class, structure, array, or other object."

"The llvm.objectsize intrinsic is lowered to a constant representing the size of the object concerned. If the size cannot be determined at compile time,llvm.objectsize returns i32/i64 -1 or 0 (depending on the min argument)."

Emphasis: "An object in this context means an allocation of a specific class, structure, array, or other object."

To me this can be clearly interpreted as a requirement to point to one "identified underlying object" in LLVM terms. A front end could now assume that if "llvm.objectsize.i32/i64" returns a value other than the failurer value it is guaranteed that the pointer value points to one underlying object and not two.

A front end could emit code like this:

ptr = phi(ptr_to_one_object, ptr_to_another_object)

if (llvm.objectsize(ptr, true) != 0) {
   // Only safe if ptr points to one object
}

If I understand correctly, your change it would break such code.

Now, you might argue that you don't like this semantics of "llvm.objectsize.i32/i64". But then, we would have to change the documentation and semantics of the intrinsic to no longer refer to "an object" but to a set of objects. There might be existing clients around that could rely on this behavior and changing the semantics of an intrinsic is something that we would have to clearly advertise, in my opinion. Not something to take lightly.


>> 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. 
>> 

See above.

Best,
Arnold