[PATCH] Add a dereferencable attribute

[Philip Reames]

I like the overall direction, but am not crazy about certain parts of the implementation.  Specially:
- the 4096 byte restriction
- is there any value to a partial pointer to a partially dereferencable pointer?  deref(2) i32* doesn't seem to make sense.  
- the consumption of so many bits in the attribute bitmask
- the documentation
- the combination of a refactoring and a functional change

================
Comment at: docs/LangRef.rst:977
@@ +976,3 @@
+    trapping. The number of bytes known to be dereferencable must be provided
+    in parentheses (only 12-bit values supported).
+
----------------
The obvious question left by the documentation is what's the difference between 'nonnull' and 'dereferencable'?  Spelling this out explicitly would be helpful.

================
Comment at: include/llvm/IR/Argument.h:65
@@ +64,3 @@
+  /// dereferencable.
+  unsigned getDereferencableBytes() const;
+
----------------
Naming here could be improved.. possibly, getBytesKnownSafeToDereference?  

The current name doesn't convey the meaning of the result.  

Also, what is returned if the attribute is not present?  Zero?  Add to the documentation.

================
Comment at: include/llvm/IR/Attributes.h:148
@@ -146,1 +147,3 @@
+  /// \brief Return true if the attribute is an integer attribute.
+  bool isIntAttribute() const;
 
----------------
This rename should be a separate change.

================
Comment at: include/llvm/IR/Attributes.h:186
@@ +185,3 @@
+  /// dereferencable attribute.
+  unsigned getDereferencableBytes() const;
+
----------------
Same issues as before. 

Since you have similar accessors throughout, pretend I repeated those comments for each.  

================
Comment at: lib/AsmParser/LLParser.cpp:1630
@@ +1629,3 @@
+  if (!EatIfPresent(lltok::kw_dereferencable))
+    return false;
+  LocTy ParenLoc = Lex.getLoc();
----------------
This doesn't seem right.  It seems like this should only be called from situations where you already have identified the dereferencable keyword.  Should this be an assert?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:688
@@ -685,3 +687,3 @@
           B.addAttribute(Kind);
         } else if (Record[i] == 1) { // Align attribute
           Attribute::AttrKind Kind;
----------------
comment update.

================
Comment at: lib/IR/Attributes.cpp:430
@@ -400,2 +429,3 @@
   case Attribute::JumpTable:       return 1ULL << 45;
+  case Attribute::Dereferencable:  return 4095ULL << 46;
   }
----------------
This encoding seems "unpleasant".  We already except string attributes from the masking scheme, would it make sense to separate integer attributes as well?  You'd need to leave handling for the existing two, but that might be least ugly option.

Also, 4096 is a fairly arbitrary limit.  I could see cases where a array passed by reference would be useful larger than this.  Why not have the units be in units of element size?  i.e. i32* dereferencable(3) would be 12 bytes.  This might make the restricted range much more powerful.  (e.g. [i32 x 20000]* dereferencable(1) is a pointer to a array of 20000 elements all of which are dereferencable.)

================
Comment at: lib/IR/Attributes.cpp:1029
@@ -980,2 +1028,3 @@
   assert((unsigned)Val < Attribute::EndAttrKinds && "Attribute out of range!");
   assert(Val != Attribute::Alignment && Val != Attribute::StackAlignment &&
+         Val != Attribute::Dereferencable &&
----------------
It might be time to extract an isIntAttrKind(...) function..

================
Comment at: lib/IR/Attributes.cpp:1138
@@ +1137,3 @@
+AttrBuilder &AttrBuilder::addDereferencableAttr(unsigned Bytes) {
+  if (Bytes == 0) return *this;
+
----------------
What happens if I try to add this twice with different sizes?  

================
Comment at: test/Transforms/LICM/hoist-deref-load.ll:4
@@ +3,3 @@
+target triple = "x86_64-unknown-linux-gnu"
+
+; This test represents the following function:
----------------
This is a pretty high level test.  Could you add a couple of smaller examples?  Actually, thinking about this a bit, that might be hard.  Feel free to ignore this comment if you can't find smaller examples.

http://reviews.llvm.org/D4449