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