[PATCH] Add support for nonnull metadata on Loads

[Hal Finkel]

----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: "Philip Reames" <listmail at philipreames.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>,
> reviews+D5220+public+15c0a768ddf63cc0 at reviews.llvm.org
> Sent: Friday, October 17, 2014 4:55:32 PM
> Subject: Re: [PATCH] Add support for nonnull metadata on Loads
> 
> ----- Original Message -----
> > From: "Nick Lewycky" <nlewycky at google.com>
> > To: reviews+D5220+public+15c0a768ddf63cc0 at reviews.llvm.org
> > Cc: "Philip Reames" <listmail at philipreames.com>, "Hal Finkel"
> > <hfinkel at anl.gov>, "Commit Messages and Patches for
> > LLVM" <llvm-commits at cs.uiuc.edu>
> > Sent: Friday, October 17, 2014 4:35:41 PM
> > Subject: Re: [PATCH] Add support for nonnull metadata on Loads
> > 
> > 
> > 
> > 
> > On 17 October 2014 14:28, Philip Reames < listmail at philipreames.com
> > >
> > wrote:
> > 
> > 
> > >>! In D5220#12, @hfinkel wrote:
> > >> I chose not to adopt the name result.nonnull. I believe we
> > >> should
> > >> stick with precedent here. There's an existing nonnull attribute
> > >> with the same semantics. I believe using an alternate name would
> > >> be more confusing.
> > > 
> > > I hate to be a pain, but -- now that I've had time to think about
> > > it ;) -- I *really* would like to name this something that makes
> > > it clear that the semantics apply to the result, and not the
> > > operand -- we *don't* currently have a naming precedent here (for
> > > metadata on loads that mirrors argument attributes), and I'd like
> > > to set a precedent that makes sense, not one that will lead to
> > > confusion. The existing attributes are clearly attached to
> > > values,
> > > so there is no ambiguity. But here there is a large ambiguity:
> > > there are two pointers involved here, and seeing !nonnull next to
> > > a load, it would be perfectly reasonable to assume that meant
> > > that
> > > the address being loaded from is nonnull (and not that the
> > > pointer
> > > being loaded is nonnull). Let's pick a less-ambiguous name and I
> > > think this is good. Let's set a precedent for result.<attr-name>
> > > or value.<attr-name> or whatever, I think that will be much
> > > better.
> > 
> > I would appreciate inputs from others on this point.
> > 
> > I don't agree with Hal's points. I don't really see there being
> > much
> > confusion - all metadata and most attributes apply to the value of
> > the instruction, not the operands - and I believe that having two
> > names for the exact same semantics (one attribute, one metadata) is
> > confusing.
> > 
> > If outvoted, I will go along with the majority here, but I believe
> > the naming scheme Hal is proposing would be a mistake.
> > 
> > 
> > 
> > An instruction *is* its result, I'm opposed to reinforcing the
> > confusion that it isn't. If you want to say something about the
> > load/store operand, use a "ptr." prefix. A weak vote for not
> > putting
> > "result." on this metadata.
> > 
> 
> Alright; valid point. Also, we likely don't want to put metadata on a
> load called 'nonnull' referring to the accessed pointer anyway. We
> might want some metadata indicating speculation safety, but that
> would have a different name ;) -- Philip, go ahead and commit (we
> can always change the name if others agree with my preference).

Actually, one more thing: You should add a constant for this metadata (LLVMContext::MD_nonnull), because we'll want to preserve it in various places easily (in r220165 for example).

 -Hal

> 
> Thanks again,
> Hal
> 
> > 
> > 
> > http://reviews.llvm.org/D5220
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> 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