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

[Arnold Schwaighofer]

On Mar 3, 2013, at 5:38 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>> Let me give an example:
>>> 
>>> %p1 = malloc(42);
>>> %p2 = malloc(42);
>>> %p3 = phi(%p1, %p2);
>>> %size = @llvm.objectsize(%p3);
>>> 
>>> The patch I commited enables instcombine to replace %size  with the
>>> constant 42.
>> 
>> Yes, and that is the problem. I claim that it is questionable whether
>> the semantics of "llvm.objectsize" does allow you to do so.
>> 
>> "The llvm.objectsize intrinsic is lowered to a constant representing
>> the size of THE object concerned"
>> - http://llvm.org/docs/LangRef.html#llvm-objectsize-intrinsic
>> 
>> There is no mentioning of more than one object. I believe you could
>> interpret this as a contract that llvm.objectsize will only return a
>> valid size value for one identified underlying object.
>> 
>> "An object in this context means AN allocation of A specific class,
>> structure, array, or other object."
>> - http://llvm.org/docs/LangRef.html#llvm-objectsize-intrinsic
>> 
>> Again, no mentioning of a set of objects. To me this reads as a clear
>> specification of what the object is. ONE and not a set.
>> 
>> If you think that the semantics of "llvm.objectsize" should be
>> different in that it allows for a set of underlying objects, again,
>> I think we have to change the LangRef. We might be breaking out of
>> tree code that interpreted the semantics as I did, though.
>> 
>> What I am concerned with is external code that took my interpretation
>> of what llvm.objectsize means:
>> 
>> Imagine a code generator taking my view and generating the following
>> code:
>> 
>> %p1 = malloc(42);
>> %p2 = malloc(42);
>> %p3 = phi(%p1, %p2);
>> %size = @llvm.objectsize(%p3, true);
>> 
>> if (%size != 0) {
>>  free(%p1)
>> } else {
>>   free(%p1)
>>   free(%p2)
>> }
> 
> I'll comment from the sidelines that, while I can see this is a theoretical possibility, we should not use it as a justification for inhibiting a useful optimization. Nevertheless, this seems to be a solvable problem. For example, we could add a third parameter to the objectsize intrinsic indicating whether ambiguous object sizes are allowed, and we could auto-upgrade the two argument form to the three argument form such that the new argument defaults to false.


Hal, I am not arguing for preventing useful optimizations. I am arguing for clarifying semantics. I will go away on the "llvm.objectsize.i32/i64" issue if the community agrees that my interpretation is wrong/not possible. Otherwise, I remain concerned what we map it to.

Yes, what you are proposing is one way of dealing with a possible change in semantics of llvm.objectsize. But as it stands, that is not what the patch we are arguing over does.