[PATCH] Add support for nonnull metadata on Loads

[Philip Reames]

ping - Hal?

On 09/08/2014 01:24 PM, Philip Reames wrote:
>
> 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
>>>>>
>>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits