[PATCH] `nonnull` argument attribute.

Mon May 19 13:59:52 PDT 2014

Nick Lewycky wrote:
> Philip Reames wrote:
>> The current state of this patch looks good to me, but I do not have
>> commit rights. Nick, could you take a look and check this in if you're
>> okay with it?

Philip politely pointed out offlist that I completely skimmed this line, 
and that I might be the only one in this thread with commit rights.

Luqman, should I commit this on your behalf or do you have commit 
access? I can make the change to Core.h myself, no need for another 
round trip.

> It may have gotten eaten by the recent email monsters, but my last
> comment was that it needed to add an entry to LLVMAttribute in
> include/llvm-c/Core.h, inside the commented-out block (sigh), but with
> that added then LGTM. I don't see that change in the phabricator, but
> assuming you add that I don't need to see the patch again. I trust you
> to make that change. :)
>
> Also there's some optimization opportunities. You can deduce 'nonnull'
> in the functionattrs pass. And this:
>
> declare void @callee(i8* nonnull)
> define void @test() {
> call @callee(i8* null)
> ret void
> }
>
> can be instcombined away.

Also to clarify, I did not intend for this to be in the same patch.

Nick

> PS. Link to previous mail ending in "With that done, please submit."
> here:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140512/216964.html
>
>
>> On 05/19/2014 10:53 AM, Luqman Aden wrote:
>>> ping
>>>
>>>
>>> On Mon, May 12, 2014 at 1:46 PM, Nick Lewycky <nicholas at mxc.ca
>>> <mailto:nicholas at mxc.ca>> wrote:
>>>
>>> Luqman Aden wrote:
>>>
>>> ping
>>>
>>>
>>> ``nonnull``
>>>  This indicates that the parameter or return pointer is not null.
>>>  This is an optimization hint to the code generator allowing it
>>> to possibly
>>>  omit some null checks. This is not checked or enforced and thus
>>> the pointer
>>>  must be guaranteed by the caller to be non-null. This parameter
>>> may only be
>>>  applied to pointer types.
>>>
>>> This is fine as is, but I'd rather you put the correctness
>>> information up front, making it:
>>>
>>> ``nonnull``
>>>  This indicates that the parameter or return pointer is not null.
>>> This attribute may only be applied to pointer typed parameters.
>>> This is not checked or enforced by LLVM, the caller must ensure
>>> that the pointer passed in is non-null, or the callee must ensure
>>> that the returned pointer is non-null.
>>>
>>> Something like that.
>>>
>>> In ValueTracking.cpp:
>>>
>>> // Is it marked not null?
>>>  if (const Argument *A = dyn_cast<Argument>(V))
>>>  return A->hasNonNullAttr();
>>> 
>>>  // A byval or inalloca argument is never null.
>>>  if (const Argument *A = dyn_cast<Argument>(V))
>>>  return A->hasByValOrInAllocaAttr();
>>> 
>>>
>>> Fine as is, but I'd prefer if you'd fold the two if-statements
>>> into one, and get:
>>>
>>> if (const Argument *A = dyn_cast<Argument>(V))
>>> return A->hasByValOrInAllocaAttr() || A->hasNonNullAttr();
>>>
>>> It should only matter in unoptimized builds.
>>>
>>> There's more you can do in instcombine, this:
>>>
>>> declare void @callee(i8* nonnull)
>>> define void @test() {
>>> call @callee(i8* null)
>>> ret void
>>> }
>>>
>>> can be optimized away to unreachable. You don't need to include
>>> that in this patch.
>>>
>>>
>>> You need to add your attribute to the commented-out block inside
>>> the definition of LLVMAttribute in include/llvm-c/Core.h .
>>>
>>> With that done, please submit.
>>>
>>> Nick
>>>
>>> On Tue, Apr 22, 2014 at 1:09 PM, Luqman Aden
>>> <me+llvm at luqman.ca <mailto:me%2Bllvm at luqman.ca>
>>> <mailto:me+llvm at luqman.ca <mailto:me%2Bllvm at luqman.ca>>> wrote:
>>>
>>> ping
>>>
>>>
>>> On Fri, Apr 18, 2014 at 9:12 PM, Philip Reames
>>> <listmail at philipreames.com <mailto:listmail at philipreames.com>
>>> <mailto:listmail at philipreames.com
>>> <mailto:listmail at philipreames.com>>> wrote:
>>>
>>> Good catch. This is a real corner case in the attribute
>>> specification. Possibly we should document the distinction
>>> somewhere?
>>>
>>> Which reminds me, we'll need doc changes (LangRef) to describe
>>> the new attribute.
>>>
>>> Philip
>>>
>>>
>>> On 04/17/2014 10:43 PM, Hal Finkel wrote:
>>>
>>> ----- Original Message -----
>>>
>>> From: "Nick Lewycky" <nicholas at mxc.ca <mailto:nicholas at mxc.ca>
>>> <mailto:nicholas at mxc.ca <mailto:nicholas at mxc.ca>>>
>>> To: reviews+D3389+public+ a85d4f162cd7a6e0 at reviews.llvm
>>> <mailto:a85d4f162cd7a6e0 at reviews.llvm>.
>>> org
>>> <mailto:reviews%2BD3389%2Bpublic%2Ba85d4f162cd7a6e0 at reviews.llvm.org
>>> <mailto:reviews%252BD3389%252Bpublic%252Ba85d4f162cd7a6e0 at reviews.llvm.org>>
>>>
>>> Cc: llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> <mailto:llvm-commits at cs.uiuc.edu
>>> <mailto:llvm-commits at cs.uiuc.edu>>
>>>
>>> Sent: Thursday, April 17, 2014 10:52:35 PM
>>> Subject: Re: [PATCH] `nonnull` argument attribute.
>>>
>>> Your change to lib/IR/Value.cpp is wrong, a nonnull
>>> pointer is not
>>> necessarily dereferenceable.
>>>
>>> %a = inttoptr i32 123456 to i32*
>>> call void @foo(i32* %a nonnull)
>>>
>>> It's nonnull, but not dereferenceable.
>>>
>>> I'll add that we really should have a dereferenceable
>>> attribute, IMHO, but as Nick said, it needs to be a separate
>>> from notnull. I think that for dereferenceable we'll want to
>>> have a more involved discussion because we'll need to decide
>>> if it affects derived pointers, should it have a size
>>> parameter, etc.
>>>
>>> -Hal
>>>
>>> Nick
>>>
>>> Luqman Aden wrote:
>>>
>>> Added test.
>>>
>>> http://reviews.llvm.org/D3389
>>>
>>> CHANGE SINCE LAST DIFF
>>> http://reviews.llvm.org/D3389? vs=8552&id=8553#toc
>>> <http://reviews.llvm.org/D3389?vs=8552&id=8553#toc
>>> <http://reviews.llvm.org/D3389?vs=8552&id=8553#toc>>
>>>
>>> Files:
>>> lib/AsmParser/LLToken.h
>>> lib/AsmParser/LLParser.cpp
>>> lib/AsmParser/LLLexer.cpp
>>> lib/Bitcode/Writer/ BitcodeWriter.cpp
>>> lib/Bitcode/Reader/ BitcodeReader.cpp
>>> lib/IR/Function.cpp
>>> lib/IR/Attributes.cpp
>>> lib/Analysis/ValueTracking.cpp
>>> test/Bitcode/attributes.ll
>>> test/Analysis/BasicAA/nonnull. ll
>>> include/llvm/Bitcode/ LLVMBitCodes.h
>>> include/llvm/IR/Attributes.h
>>> include/llvm/IR/Argument.h
>>>
>>>
>>>
>>> ______________________________ _________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> <mailto:llvm-commits at cs.uiuc.edu
>>> <mailto:llvm-commits at cs.uiuc.edu>>
>>> http://lists.cs.uiuc.edu/
>>> mailman/listinfo/llvm-commits
>>> <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>>>
>>> ______________________________ _________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> <mailto:llvm-commits at cs.uiuc.edu
>>> <mailto:llvm-commits at cs.uiuc.edu>>
>>> http://lists.cs.uiuc.edu/ mailman/listinfo/llvm-commits
>>> <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>>>
>>>
>>> ______________________________ _________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> <mailto:llvm-commits at cs.uiuc.edu
>>> <mailto:llvm-commits at cs.uiuc.edu>>
>>> http://lists.cs.uiuc.edu/ mailman/listinfo/llvm-commits
>>> <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>