[PATCH] `nonnull` argument attribute.
Nick Lewycky
nicholas at mxc.ca
Mon May 19 13:44:04 PDT 2014
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?
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.
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
>
More information about the llvm-commits
mailing list