[PATCH] Add support for nonnull metadata on Loads

[Philip Reames]

On 09/17/2014 05:19 PM, Hal Finkel wrote:
> ----- Original Message -----
>> From: "Philip Reames" <listmail at philipreames.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: llvm-commits at cs.uiuc.edu, reviews+D5220+public+15c0a768ddf63cc0 at reviews.llvm.org
>> Sent: Wednesday, September 17, 2014 6:56:03 PM
>> Subject: Re: [PATCH] Add support for nonnull metadata on Loads
>>
>> ping - Hal?
> I'd still like to conduct some overhead experiments with using value attributes for this, but there is no need to hold up this functionality in the mean time (we can always auto-upgrade if we decide to go that route).
>
> As a general note, if we do this, I'd also like corresponding metadata for the dereferenceable(<n>) (and maybe also align <n>, although that is also representable using @llvm.assume) [not that you need to do that yourself].
Agreed.  Future work for someone.
>   I'll take you up on your offer to do the @llvm.assume canonicalization, however.
Yep, no problem.  I plan on only handling the trivial case of an assume 
in the same basic block as the load FYI.  Otherwise, I've got to deal 
with path conditions that aren't obvious.  FYI, this will probably end 
up submitted separately, but I do plan to do it.
> Also, you need to update the various places that drop/merge metadata on loads to deal with this metadata (GVN, vectorizers, etc. and llvm::combineMetadata).
I'll do these as separate patches.  Dropping the metadata is sound, and 
the current form has benefits the cases I care about.
>
> You obviously also need LangRef updates.
Ack.  Do you want to review those before hand?
>
>   -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
>>