[PATCH] Add support for nonnull metadata on Loads

[Hal Finkel]

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

Thanks again,
Hal

> 
> 
> http://reviews.llvm.org/D5220
> 

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