[PATCH] Add a dereferencable attribute

[Hal Finkel]

----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: "Bill Wendling" <isanbard at gmail.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>, "reviews+d4449+public+728896e6e6399a87"
> <reviews+D4449+public+728896e6e6399a87 at reviews.llvm.org>
> Sent: Thursday, July 10, 2014 5:52:31 PM
> Subject: Re: [PATCH] Add a dereferencable attribute
> 
> Bill,
> 
> Can you provide advise on the approach here? I'd like to add
> parameter attribute named dereferenceable that takes a size. Is
> there a way to do this without grabbing a lot of bits from the
> current attribute bitfield? The alignment attributes grab bits from
> the field, but they need only a few. Although the current patch uses
> 12 bits, that seems like both a lot (relative to the number of free
> bits remaining) and too little (one could easily want more).

Looking at the bitcode reader/writer, correct me if I'm wrong, but the bitcode always uses a 64-bit record for every attribute, so the bitcode is actually not the problem. The apparent problem comes from the handling of the old PARAMATTR_CODE_ENTRY_OLD blocks, and I think that we can probably do without supporting a new attribute in the old format.

 -Hal

> 
> Thanks again,
> Hal
> 
> ----- Original Message -----
> > From: "Hal Finkel" <hfinkel at anl.gov>
> > To: "Nick Lewycky" <nlewycky at google.com>
> > Cc: "Commit Messages and Patches for LLVM"
> > <llvm-commits at cs.uiuc.edu>,
> > "reviews+d4449+public+728896e6e6399a87"
> > <reviews+D4449+public+728896e6e6399a87 at reviews.llvm.org>
> > Sent: Thursday, July 10, 2014 5:11:29 PM
> > Subject: Re: [PATCH] Add a dereferencable attribute
> > 
> > ----- Original Message -----
> > > From: "Nick Lewycky" <nlewycky at google.com>
> > > To: "reviews+d4449+public+728896e6e6399a87"
> > > <reviews+D4449+public+728896e6e6399a87 at reviews.llvm.org>
> > > Cc: "Hal Finkel" <hfinkel at anl.gov>, "Chandler Carruth"
> > > <chandlerc at gmail.com>, "aschwaighofer"
> > > <aschwaighofer at apple.com>, "Philip Reames"
> > > <listmail at philipreames.com>, "me+llvm" <me+llvm at luqman.ca>,
> > > "Commit
> > > Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> > > Sent: Thursday, July 10, 2014 5:06:08 PM
> > > Subject: Re: [PATCH] Add a dereferencable attribute
> > > 
> > > 
> > > 
> > > 
> > > On 10 July 2014 14:13, hfinkel at anl.gov < hfinkel at anl.gov > wrote:
> > > 
> > > 
> > > Updated per Nick's comment to include an explicit size (number of
> > > bytes). I currently picked 12 bits for the size, I'm not sure if
> > > that is a good choice (I did not want to use too many bits, are
> > > there only 64 in total?).
> > > 
> > > Also, I renamed the AlignAttr class to IntAttr (along with all
> > > associated functions) because these are now just integer-carrying
> > > attributes (not specifically some kind of alignment).
> > > 
> > > 
> > > http://reviews.llvm.org/D4449
> > > 
> > 
> > Nick, do you know if there is a better way to do the size than
> > taking
> > up 12 bits in the attribute field? It seems both like a lot and not
> > enough ;)
> > 
> > > 
> > > "dereferencable" vs. "dereferenceable". I think LLVM is
> > > consistently
> > > using the latter right now, your patch consistently uses the
> > > former.
> > > Please fix, either way.
> > 
> > I'll change the patch.
> > 
> > > 
> > > 
> > > You know how we never print "readonly readnone" because instead
> > > we
> > > just print "readnone"? (This is reflected in the API as well,
> > > querying for "onlyReadsMemory" returns true if readnone is set
> > > and
> > > readonly isn't, or if readonly is.) What do you think of doing
> > > this
> > > for "nonnull dereferencable(n)"? The only gotcha is that it only
> > > works when the pointer is in address space zero, in non-zero
> > > address
> > > spaces dereferenceable(n) does not imply nonnull.
> > > 
> > > 
> > 
> > That's a good idea. Dereferenceable implied nonnull in
> > addressspace(0).
> > 
> > Thanks again,
> > Hal
> > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > Files:
> > > docs/LangRef.rst
> > > include/llvm-c/Core.h
> > > include/llvm/Bitcode/LLVMBitCodes.h
> > > include/llvm/IR/Argument.h
> > > include/llvm/IR/Attributes.h
> > > include/llvm/IR/CallSite.h
> > > include/llvm/IR/Function.h
> > > include/llvm/IR/Instructions.h
> > > lib/AsmParser/LLLexer.cpp
> > > lib/AsmParser/LLParser.cpp
> > > lib/AsmParser/LLParser.h
> > > lib/AsmParser/LLToken.h
> > > lib/Bitcode/Reader/BitcodeReader.cpp
> > > 
> > > 
> > > 
> > > 
> > > else if (Kind == Attribute::Dereferencable)
> > > B.addDereferencableAttr(Record[++i]);
> > > else
> > > llvm_unreachable("Unknown integer attribute type");
> > > 
> > > 
> > > I'm not sure that the appropriate use of llvm_unreachable. It can
> > > be
> > > reached with a crafted .bc file, right?
> > > 
> > > 
> > > lib/Bitcode/Writer/BitcodeWriter.cpp
> > > lib/IR/AttributeImpl.h
> > > 
> > > 
> > > lib/IR/Attributes.cpp
> > > 
> > > 
> > > 
> > > unsigned AttributeSetNode::getDereferencableBytes() const {
> > > for (iterator I = begin(), E = end(); I != E; ++I)
> > > 
> > > 
> > > Range-based for loop?
> > > 
> > > 
> > > 
> > > 
> > > lib/IR/Function.cpp
> > > lib/IR/Value.cpp
> > > test/Bitcode/attributes.ll
> > > test/Transforms/LICM/hoist-deref-load.ll
> > > 
> > 
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

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