[PATCH] Add support for nonnull metadata on Loads

Thu Sep 18 10:24:35 PDT 2014

----- 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: Thursday, September 18, 2014 12:14:10 PM
> Subject: Re: [PATCH] Add support for nonnull metadata on Loads
> 
> 
> 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.

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.

> > 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.

> >
> > You obviously also need LangRef updates.
> Ack.  Do you want to review those before hand?

Yes, we need the LangRef updates with the feature.

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
> >>
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory