[PATCH] Add support for nonnull metadata on Loads

[Philip Reames]

On 09/08/2014 01:17 PM, Hal Finkel wrote:
> ----- Original Message -----
>> From: "Philip Reames" <listmail at philipreames.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>, reviews+D5220+public+15c0a768ddf63cc0 at reviews.llvm.org
>> Cc: llvm-commits at cs.uiuc.edu
>> Sent: Monday, September 8, 2014 1:21:45 PM
>> Subject: Re: [PATCH] Add support for nonnull metadata on Loads
>>
>>
>> On 09/08/2014 04:32 AM, Hal Finkel wrote:
>>> ----- Original Message -----
>>>> From: "Philip Reames" <listmail at philipreames.com>
>>>> To: listmail at philipreames.com, hfinkel at anl.gov
>>>> Cc: llvm-commits at cs.uiuc.edu
>>>> Sent: Friday, September 5, 2014 4:42:25 PM
>>>> Subject: [PATCH] Add support for nonnull metadata on Loads
>>>>
>>>> Hi hfinkel,
>>>>
>>>> This patch simply adds support for marking a load as returning a
>>>> non-null pointer.  This is analogous to the existing nonnull
>>>> return
>>>> attribute, but it applies to specific load instructions instead.
>>>>    The value of the load is allowed to vary between successive
>>>>    loads,
>>>> but null is not a valid value to be loaded by any load marked
>>>> nonnull.
>>>>
>>>> Hal - I'd particularly like your input here.  Is adding a parallel
>>>> metadata construct to the existing attribute a reasonable idea?
>>>>    Should we be uniform and accept the metadata on calls too?  Is
>>>> there a better way to approach this.
>>> Now that the @llvm.assume infrastructure is in place, I can
>>> legitimately respond by saying: I think that we can model this by
>>> @llvm.assume(x != null). If that currently does not work, then we
>>> should fix it.
>> A couple of counter points:
>> 1) We already use metadata for optimization hints in multiple ways.
>> It
>> works.  While having this represented via assumes is nice, I don't
>> want
>> to block forward progress in a search for perfection.
>> 2) I don't think assumes and metadata should be considered an
>> either-or.  If something is widely useful enough, having it supported
>> by
>> metadata is actually a good thing.  As we've discussed previously,
>> using
>> assumes has some downsides.
>> 3) If anything, having assume(x != null) canonicalize to a nonnull
>> attribute or metadata (depending on the type of x), would seem like a
>> very reasonable implementation.
> I agree with all of this, but let's not prematurely optimize. The assume technique should work now, and if it becomes clear that using it has too much overhead, then using metadata seems like the next logical choice.
Hal, I'm honestly not really getting your objection here.  The cost of 
an extra bit of metadata is tiny.  The patch is clean and small. It 
doesn't contradict any of the long term plans you've mentioned.

On a more practical side, I'm happy to add (3) to my original patch, but 
I am unlikely to go any further.  I'd prefer not to maintian this patch 
in my private branch, but I'm not willing to invest major effort in an 
alternate implementation to avoid that (very minimal) effort.  If you'd 
prefer not to see the metadata approach land at all, please say so.  
I'll move on with other projects.

>
>   -Hal
>
>> Philip
>>>> http://reviews.llvm.org/D5220
>>>>
>>>> Files:
>>>>     lib/Analysis/ValueTracking.cpp
>>>>     test/Transforms/InstSimplify/compare.ll
>>>>
>>