[PATCH] Add support for nonnull metadata on Loads

Philip Reames listmail at philipreames.com
Thu Sep 18 11:15:43 PDT 2014 

On 09/18/2014 10:24 AM, Hal Finkel wrote:
> ----- Original Message -----
>> From: "Philip Reames" <listmail at philipreames.com>
>>>    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.
> You should iterate over all assumptions, looking for those providing nonnull assumptions on loads (after stripping bitcasts). Regarding applicability, you can re-use the existing logic for this. Call isValidAssumeForContext (from ValueTracking) providing the assumption and the load.
I think you're picturing something different than I am.  I was planning 
(for the moment) to exactly match the pattern:
%p = load ...
p != nullptr <-- immediately after in the same basic block
assume() <-- immediately after in the same basic block

Stripping bitcasts isn't needed.  This will be done by other parts of 
instcombine.

This gets the simplest cases.  I'm also not planning to actually remove 
the assume, just add the metadata.  This ties into your point below.

Looking at isValidAssumeForContext, that would involve walking all the 
uses of the load.  I thought generally preferred not to work "forward" 
in instcombine?

An alternate approach would be a stronger "if all non ephemeral uses are 
dominated by assume" condition, but that again involves working forward 
from the load.

I think it's worth handling the simplest possible case first, then 
coming back to the others.

>
>>> 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.
> That's fine. This needs to happen before the assume canonicalization, however.
If I was planning on removing the assume, yes.  Since I'm not, there's 
no harm in eagerly applying the metadata even if it gets dropped 
repeatedly.  (It could be a performance issue, but I doubt it.)
>
>>> You obviously also need LangRef updates.
>> Ack.  Do you want to review those before hand?
> Yes, we need the LangRef updates with the feature.
Diff updated.  Note that I haven't actually built this.  I've never 
gotten sphinx working on my local machine.
>
> Thanks again,
> Hal
>
>>>    -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
>>

More information about the llvm-commits mailing list