[PATCH] `nonnull` argument attribute.

Mon May 19 14:50:42 PDT 2014

On Mon, May 19, 2014 at 1:59 PM, Nick Lewycky <nicholas at mxc.ca> wrote:

> Nick Lewycky wrote:
>

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

I don't have commit access so it'd be great if you could commit it. Also,
I'm pretty sure I made the change to Core.h. Adding a new variant to
LLVMAttribute right? It seems to show in phabricator for me.

>
>
>  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
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140519/cafe9db2/attachment.html>