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

[Arnold Schwaighofer]

Hi Shuxin,

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

> Hi, Arnold:
> 
>  You are right. As I put in the report to the pr14988, my concerns have two parts:
>   1) in alias analysis context, while Nuno's change dose not seem correctness problem for what we have today.
>       It likely has problem in the future, as it deviate the original meaning of class ObjectSizeOffsetVisitor.
>       I gave a contrived problem in the previous mail.
> 

I think we are mostly in wild agreement here :).

I want APIs with clearly defined semantics as do you.

I am saying if we want Nuno's functionality we (he ;) will have to clean up code:

Introduce several functions and use them appropriately, possibly rename 
ObjectSizeOffsetVisitor to ObjectsSizeOffsetVisitor (or something similar), etc . This will prevent any future confusion.

Future code will have to respect exising code or use different functions: getUnderlyingObjectSize vs getUnderlyingObjectsSize, etc.

By introducing a clearly defined API, future code will be aware that there is a difference and bugs that could arise because of unawareness can be prevented.


>       What you proposed here is that : if Nuno's change have to be committed, we can remedy this concern by
>       coming up another set of functions, each having *CLEAR* meaning, and semantics.
> 

Yes.

>       That is absolutely right, but I don't
> think it's worth that trouble.

I think us arguing about what the semantics of things should be - is a clear indicator that the sematics is unclear and as such it is worth the trouble.

If Nuno thinks it is worth the trouble I am not for standing in his way.

> 
>   2) in the context llvm.objectsize related optimization.  I'm not expert of the llvm lang-ref.
>        However, IMHO, even if it has some loophole in the lang spec, we should not take this advantage to
>        code hard to read or maintain.


 I am not arguing for making code hard to read or maintain. I argue for the exact opposite. Define an API with clearly defined semantics and clarify the semantics of "llvm.objectsize.i32/i64" in the LangRef.

There is no loophole in the language spec. I just have a question of which of the following functions can "llvm.objectsize.i32/i64" be mapped to

Can it be

> getSizeOfPointerAdjustedUnderlyingObjects

or must it be

> getSizeOfPointerAdjustedUniqueUnderlyingObject

I think the existing text in the LangRef.html makes this a least questionable. If it must be the second then Nuno's patch is wrong (possibly also existing code before his patch as he mentioned that the existing code already handles select).


Best,
Arnold