[PATCH] Add a dereferencable attribute

[hfinkel at anl.gov]

----- Original Message -----
> From: "Philip Reames" <listmail at philipreames.com>
> To: hfinkel at anl.gov, chandlerc at gmail.com, aschwaighofer at apple.com, nlewycky at google.com, me+llvm at luqman.ca,
> listmail at philipreames.com
> Cc: "james molloy" <james.molloy at arm.com>, llvm-commits at cs.uiuc.edu
> Sent: Thursday, July 10, 2014 5:00:15 PM
> Subject: Re: [PATCH] Add a dereferencable attribute
> 
> I like the overall direction, but am not crazy about certain parts of
> the implementation.  Specially:
> - the 4096 byte restriction

I'm not crazy about this either.

> - is there any value to a partial pointer to a partially
> dereferencable pointer?  deref(2) i32* doesn't seem to make sense.

As Nick pointed out initially, this actually does make sense. Many targets fool with the actual parameter types for ABI reasons (including "padding" them), and it is important to be able to specify that only part of the type size is dereferenceable.

> - the consumption of so many bits in the attribute bitmask

Yep.

> - the documentation

Okay.

> - the combination of a refactoring and a functional change

There really isn't any (aside from the one renaming, which I can certainly commit separately).

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

Agreed.

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

Yea, I'm happy to commit this separately.

> 
> ================
> 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?

This is done in exactly the same way as the other parametrized attributes. Maybe all of them need some work, but that's a separate issue.

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

Thanks.

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

I'm not sure, and as I recall, we don't actually accept string attributes on parameters, so some more-general changes would be needed.

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

You don't need to convince me to not like the limit, I don't like it. Someone with a better understanding of the bitcode format, I hope, will be able to suggest something better.

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

Agreed.

> 
> ================
> 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?

The same thing as happens for an alignment (it replaces the previous one, IIRC).

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

I don't know of any either.

Thanks again,
Hal

> 
> http://reviews.llvm.org/D4449
> 
> 
>

http://reviews.llvm.org/D4449