Attributes: add a new allocalign attribute

[durin42]

Summary

This will let us start moving away from hard-coded attributes in
MemoryBuiltins.cpp and put the knowledge about various attribute
functions in the compilers that emit those calls where it probably
belongs.

[nikic]

Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.

If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.

[reames]

Cannot review without LangRef changes.

[durin42]

In D117921#3262394, @nikic wrote:

Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.

I don't have any idea. @reames any opinion? I confess I'm not sure what this means for me.

If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.

I can't do it zero-based, because there's no way to store a zero in an int attribute. I can go to the work of adding getters and setters everywhere and then pretend it's 0-based for the public API, but internally you'll fail assertions if you try to carry 0 as the payload of an int attr. If you want the getters and setters let me know, I guess? This will matter for one other attribute we need too (mustfree(N) - to indicate a pointer will be freed).

In D117921#3262397, @reames wrote:

Cannot review without LangRef changes.

Done.

[reames]

In D117921#3269497, @durin42 wrote:

In D117921#3262394, @nikic wrote:

Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.

I don't have any idea. @reames any opinion? I confess I'm not sure what this means for me.

I don't see anyway with argument align attributes to express the alignment of the return value of the call. Specifically, the fact this is trying to express is "the returned pointer has at least the alignment of the integer param X".

If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.

I can't do it zero-based, because there's no way to store a zero in an int attribute. I can go to the work of adding getters and setters everywhere and then pretend it's 0-based for the public API, but internally you'll fail assertions if you try to carry 0 as the payload of an int attr. If you want the getters and setters let me know, I guess? This will matter for one other attribute we need too (mustfree(N) - to indicate a pointer will be freed).

Er, this seems strongly suspect to me. I have not looked carefully, but I'm pretty sure we do use zero based indexing in allocsize. You should be able to parallel that structure. (You don't need the argument packing bit, but at the same time it does no harm to reuse if it simplifies the code.)

You need tests for this. Both IR, and bitcode parsing.

I would suggest focusing this change on the mechanics of round tripping the new attribute and defining it. We can defer the use to a separate patch.

llvm/docs/LangRef.rst

1554

This definition needs expanded.

Questions:

• Units?
• one argument, two arguments?
• assumptions about new memory?

See the alllocsize for example text.

llvm/lib/Analysis/MemoryBuiltins.cpp

227 ↗

(On Diff #402904)

Nope, would be same place we handle allocsize today.

339 ↗

(On Diff #402904)

To parallel the allocsize path, we need builtin knowledge to overrule attributed knowledge.

[durin42]

In D117921#3269526, @reames wrote:

In D117921#3269497, @durin42 wrote:

In D117921#3262394, @nikic wrote:

If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.

I can't do it zero-based, because there's no way to store a zero in an int attribute. I can go to the work of adding getters and setters everywhere and then pretend it's 0-based for the public API, but internally you'll fail assertions if you try to carry 0 as the payload of an int attr. If you want the getters and setters let me know, I guess? This will matter for one other attribute we need too (mustfree(N) - to indicate a pointer will be freed).

Er, this seems strongly suspect to me. I have not looked carefully, but I'm pretty sure we do use zero based indexing in allocsize. You should be able to parallel that structure. (You don't need the argument packing bit, but at the same time it does no harm to reuse if it simplifies the code.)

allocsize is different because it has two parameters, one optional. It's not possible for both to be set to zero, so the payload it carries will _never_ be zero. It does use zero-based indexing, but we'll have to do custom getters/settings across the board like allocsize if we want to use zero-based indexing in both allocalign and mustfree. If that's the preference, I'm happy to do it, but it does seem like a fair amount of extra code for only marginal benefit.

[nikic]

In D117921#3269526, @reames wrote:

In D117921#3269497, @durin42 wrote:

In D117921#3262394, @nikic wrote:

Might it make more sense to implement this as an argument attribute? Would be a bit simpler I think, and I don't really see a disadvantage.

I don't have any idea. @reames any opinion? I confess I'm not sure what this means for me.

I don't see anyway with argument align attributes to express the alignment of the return value of the call. Specifically, the fact this is trying to express is "the returned pointer has at least the alignment of the integer param X".

To clarify, what I mean is:

; Instead of
declare i8* @aligned_alloc(i64, i64) allocalign(1)
; You do
declare i8* @aligned_alloc(i64, i64 allocalign)

That is, the allocalign attribute indicates that the value provided to this parameter will the alignment of the return value. The semantics would be the same as with the return attribute, you just don't need to deal with integer attributes.

If we do want to use the function attribute, I think it should be possible to support proper zero values for integer attributes.

[jyknight]

That is, the allocalign attribute indicates that the value provided to this parameter will the alignment of the return value. The semantics would be the same as with the return attribute, you just don't need to deal with integer attributes.

Using a parameter attribute instead of a function attribute with a parameter number seems quite reasonable. Let's go with that.

If we do want to use the function attribute, I think it should be possible to support proper zero values for integer attributes.

I don't think this would be straightforward. The attribute system has the assumption that "unset == 0" in many parts of its API.

[jyknight]

llvm/docs/LangRef.rst

1554

I don't think the allocsize description is a great example of clarity...

For allocsize, I'd want to know how it relates to -- or differs from -- placing dereferenceable_or_null(value-of-EltSizeParam) or dereferenceable_or_null(value-of-EltSizeParam * value-of-NumEltsParam) on the function return. Other than that the latter is only expressible when the values are constants, of course.

It is not at all clear from the wording, but it's treated completely differently internally right now. (Maybe it shouldn't actually be? Dunno! The current description doesn't seem to suggest any of the semantics we've actually applied to it!)

Anyways, for allocalign, I think we can probably do better.

E.g, maybe we can say "This attribute is equivalent to placing align(value-of-allocalign-parameter) on the function return, except that the value is not required to be a constant, and <something about non-power-of-2 values>." -- and actually have the code treat it that way, as well.

So, what _should_ the semantics be if the parameter is not a power-of-2?

• UB.
• Ignore (treat as if alignment was unknown -- e.g. 1)
• Something else?

Looks like we're currently inconsistent in the handling at the moment for the hardcoded functions. AttributorAttributes.cpp seems to be assuming power-of-2 (and tries to create an invalid alloca instruction if it's not), while InstCombineCalls.cpp ignores the specified alignment if it's not a power-of-2. (Am I the only one surprised that "Attributor" can make very-much non-attribute IR changes like doing heap-to-stack!)

I think I'd vote for "ignore".

Units: Yea, clearly this is "alignment in bytes", but that should be specified.

llvm/lib/Analysis/MemoryBuiltins.cpp

339 ↗

(On Diff #402904)

Why does allocsize work that way? I'd generally expect an attribute to take precedence if specified?

[durin42]

llvm/lib/Analysis/MemoryBuiltins.cpp

339 ↗

(On Diff #402904)

I'm new to compilers at all and LLVM in particular, but I really would have expected an attribute to overrule builtin knowledge too, to the point that I didn't check how allocsize was set up. For now I'm making it consistent, but it does seem backwards.

Also, the "use the attribute" part of this has moved to a new change.

[reames]

In D117921#3269537, @durin42 wrote:

It does use zero-based indexing, but we'll have to do custom getters/settings across the board like allocsize if we want to use zero-based indexing in both allocalign and mustfree. If that's the preference, I'm happy to do it, but it does seem like a fair amount of extra code for only marginal benefit.

If the existing infrastructure truly doesn't allow zero as a value for an attribute as an expressability level, then it's over specialized for existing attributes, and we should generalize.

With @nikic's clarification, I now understand what he meant, and agree that looks reasonable as well.

llvm/lib/Analysis/MemoryBuiltins.cpp

339 ↗

(On Diff #402904)

I have no idea why the code is structured the current way, but having the two *differ* seems like a recipe for confusion and disaster.

If you want to change the way the existing attribute is handled, you could do the research and work through that.

If you want to leave the order the same for the two, and add a TODO for latter work on reversing it, I'm also fine. Note that order doesn't matter if the baseline knowledge is removed as we seem to be working towards.

[nikic]

llvm/docs/LangRef.rst

1552

This shouldn't be in the "function attributes" section.

1553

"The function parameter marked with this attribute is [...]" maybe? The current phrasing reads odd to me.

1554

I would probably go for UB here, in the interest of allowing handling of non-constant alignments by creating assumes. LangRef is not clear on this, but I expect that an align operand bundle assume with a non-power-of-two value would be UB.

No strong preference though.

[reames]

This patch looks mechanically fine, we just need to get the LangRef wording finalized.

I did have a thought when reading over this. Is there anything specific here to an allocation function? Could we generalize this into a generic "retalign" parameter attribute which generalizes align(N) on the return value and allows non-constant alignments to be described?

I'm not sure that generalization is actually useful, it just seems like we have something which is almost non-allocation specific. If we could think of examples where non-constant alignments are useful, then it seems a shame to leave it specialized...

llvm/docs/LangRef.rst

1383

I can't make heads or tails of the current wording. I'd suggest something along the lines of:

This attribute indicates that the alignment of the newly allocated object returned by this allocation function can be assumed to be greater than the runtime value of the marked parameter.

That is, it's similar to putting`align(N)` on the return value of the function but allows the alignment to be a runtime value.

Runtime alignment values which aren't a power of 2 are undefined behavior.

[jyknight]

So, sorry for not noticing this sooner, but...I just realized we actually already have a way to spell something very much like this already, with llvm.assume.

%ret = call i8* @aligned_alloc(i64 %0, i64 %1)
call void @llvm.assume(i1 true) [ "align"(i8* %ret, i64 %0) ]

Maybe we _shouldn't_ be adding an attribute for this, given the above, and rather just insert llvm.assume in appropriate places? @jdoerfert maybe has an opinion here?

llvm/docs/LangRef.rst

1554

On second look, this must NOT be UB.

Per C2x aligned_alloc:
"If the value of alignment is not a valid alignment supported by the implementation the function shall fail by returning a null pointer." (and elsewhere it says "Every valid alignment value shall be a nonnegative integral power of two.")

We cannot turn a guarantee of returning nullptr into UB. We must be prepared to accept any alignment at runtime, even if we ignore non-power-of-2 ones.

[nikic]

In D117921#3294002, @reames wrote:

This patch looks mechanically fine, we just need to get the LangRef wording finalized.

I did have a thought when reading over this. Is there anything specific here to an allocation function? Could we generalize this into a generic "retalign" parameter attribute which generalizes align(N) on the return value and allows non-constant alignments to be described?

I'm not sure that generalization is actually useful, it just seems like we have something which is almost non-allocation specific. If we could think of examples where non-constant alignments are useful, then it seems a shame to leave it specialized...

I agree that the actual semantics here should be independent of whether the function is also, in some sense, considered an "allocator". I'd be fine still calling it allocalign in that case (similar to how allocsize is a general statement that the number of bytes is dereferenceable_or_null). retalign also sounds reasonable though.

llvm/docs/LangRef.rst

1554

Good point. In that case I'm fine with saying that non-power-of-two alignment is ignored. Alternatively we could specify that ptrtoint(ret) % param == 0, which would be the non-power-of-two generalization (and trivially holds for the null pointer), but I don't think there is anything useful we could do with that, and it's probably best to stick with the well-defined notion of alignment (don't want to bring question like "what about non-integral pointers?" into this.)

[jyknight]

In D117921#3294602, @jyknight wrote:

So, sorry for not noticing this sooner, but...I just realized we actually already have a way to spell something very much like this already, with llvm.assume.

%ret = call i8* @aligned_alloc(i64 %0, i64 %1)
call void @llvm.assume(i1 true) [ "align"(i8* %ret, i64 %0) ]

Maybe we _shouldn't_ be adding an attribute for this, given the above, and rather just insert llvm.assume in appropriate places? @jdoerfert maybe has an opinion here?

After thinking about this a bit more, I think I've actually convinced myself that introducing the attribute is the right thing to do after all. We'll need to make sure to document this well, of course...

The semantics of llvm.assume is only that the return value must be aligned _at least_ to the given amount. If you see a @llvm.assume(i1 true) [ "align"(i8* %ret, i64 2) ] that means you know that %ret is aligned to at least 2-bytes. However, "an alignment assumption which is true at this point" is insufficient to be able to correctly do heap->stack conversion of an allocation call. For that, we must know the alignment which was required of that allocation call.

It's semantically correct to lower the alignment value on -- or entirely discard -- an llvm.assume "align" value, but it is NOT valid to use a lower alignment than was requested for an allocation call.

I think, thus, we can say that "allocalign" implies the llvm.assume "align", in terms of what a caller can assume about the return value's alignment, but also has the additional semantics of indicating the requested alignment of the allocation, which the compiler may use if it decides to elide an allocation. (This also implies that generalizing this attribute to "retalign" is not desirable nor necessary.)

[jyknight]

I'm happy with the LangRef semantics and wording that here now.

[nikic]

llvm/docs/LangRef.rst

1388

Should we specify that the attribute may only be applied to integer parameters, and add a corresponding rule to typeIncompatible()?

llvm/lib/Transforms/Utils/CodeExtractor.cpp

905

This should be in the bottom category (non-function attrs).

[durin42]

llvm/docs/LangRef.rst

1388

Sure, done.

llvm/lib/Transforms/Utils/CodeExtractor.cpp

905

Fixed, thanks.

[nikic]

LGTM

llvm/docs/LangRef.rst

``alignstack(<n>)``
     This indicates the alignment that should be considered by the backend when
     assigning this parameter to a stack slot during calling convention
     lowering. The enforcement of the specified alignment is target-dependent,
     as target-specific calling convention rules may override this value. This
     attribute serves the purpose of carrying language specific alignment
     information that is not mapped to base types in the backend (for example,
     over-alignment specification through language attributes).
 
+``allocalign``
+    The function parameter marked with this attribute is is the alignment in bytes of the
+    newly allocated block returned by this function. That is, it's similar to putting
+    `align(N)` on the return value of the function but it's specified by the caller of the
+    function and values which aren't a power of 2 are undefined behavior.
+
 .. _gc:
 
 Garbage Collector Strategy Names
 --------------------------------
 
 Each function may specify a garbage collector strategy name, which is simply a
 string:
 
 .. code-block:: llvm
.. code-block:: llvm
     define void @f() alwaysinline optsize { ... }
     define void @f() optsize { ... }
 
 ``alignstack(<n>)``
     This attribute indicates that, when emitting the prologue and
     epilogue, the backend should forcibly align the stack pointer.
     Specify the desired alignment, which must be a power of two, in
     parentheses.
 ``allocsize(<EltSizeParam>[, <NumEltsParam>])``
     This attribute indicates that the annotated function will always return at
     least a given number of bytes (or null). Its arguments are zero-indexed
     parameter numbers; if one argument is provided, then it's assumed that at
     least ``CallSite.Args[EltSizeParam]`` bytes will be available at the
     returned pointer. If two are provided, then it's assumed that
     ``CallSite.Args[EltSizeParam] * CallSite.Args[NumEltsParam]`` bytes are
     available. The referenced parameters must be integer types. No assumptions
     are made about the contents of the returned block of memory.
 ``alwaysinline``
     This attribute indicates that the inliner should attempt to inline

llvm/include/llvm/AsmParser/LLToken.h

enum Kind {
   kw_amdgpu_ps,
   kw_amdgpu_cs,
   kw_amdgpu_kernel,
   kw_amdgpu_gfx,
   kw_tailcc,
 
   // Attributes:
   kw_attributes,
+  kw_allocalign,
   kw_allocsize,
   kw_alwaysinline,
   kw_argmemonly,
   kw_sanitize_address,
   kw_sanitize_hwaddress,
   kw_sanitize_memtag,
   kw_builtin,
   kw_byval,

llvm/include/llvm/Bitcode/LLVMBitCodes.h

enum AttributeKindCodes {
   ATTR_KIND_NO_CALLBACK = 71,
   ATTR_KIND_HOT = 72,
   ATTR_KIND_NO_PROFILE = 73,
   ATTR_KIND_VSCALE_RANGE = 74,
   ATTR_KIND_SWIFT_ASYNC = 75,
   ATTR_KIND_NO_SANITIZE_COVERAGE = 76,
   ATTR_KIND_ELEMENTTYPE = 77,
   ATTR_KIND_DISABLE_SANITIZER_INSTRUMENTATION = 78,
+  ATTR_KIND_ALLOC_ALIGN = 79,
 };
 
 enum ComdatSelectionKindCodes {
   COMDAT_SELECTION_KIND_ANY = 1,
   COMDAT_SELECTION_KIND_EXACT_MATCH = 2,
   COMDAT_SELECTION_KIND_LARGEST = 3,
   COMDAT_SELECTION_KIND_NO_DUPLICATES = 4,
   COMDAT_SELECTION_KIND_SAME_SIZE = 5,

llvm/include/llvm/IR/Attributes.td

 class StrBoolAttr<string S> : Attr<S, []>;
 
 /// Target-independent enum attributes.
 
 /// Alignment of parameter (5 bits) stored as log2 of alignment with +1 bias.
 /// 0 means unaligned (different from align(1)).
 def Alignment : IntAttr<"align", [ParamAttr, RetAttr]>;
 
+/// Parameter of a function that tells us the alignment of an allocation, as in 
+/// aligned_alloc and aligned ::operator::new.
+def AllocAlign: EnumAttr<"allocalign", [ParamAttr]>;
+
 /// The result of the function is guaranteed to point to a number of bytes that
 /// we can determine if we know the value of the function's arguments.
 def AllocSize : IntAttr<"allocsize", [FnAttr]>;
 
 /// inline=always.
 def AlwaysInline : EnumAttr<"alwaysinline", [FnAttr]>;
 
 /// Function can access memory only using pointers based on its arguments.

llvm/lib/AsmParser/LLLexer.cpp

#define KEYWORD(STR)                                                           \
   KEYWORD(tailcc);
 
   KEYWORD(cc);
   KEYWORD(c);
 
   KEYWORD(attributes);
 
   KEYWORD(alwaysinline);
+  KEYWORD(allocalign);
   KEYWORD(allocsize);
   KEYWORD(argmemonly);
   KEYWORD(builtin);
   KEYWORD(byval);
   KEYWORD(inalloca);
   KEYWORD(cold);
   KEYWORD(convergent);
   KEYWORD(dereferenceable);

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
   case bitc::ATTR_KIND_NON_LAZY_BIND:
     return Attribute::NonLazyBind;
   case bitc::ATTR_KIND_NON_NULL:
     return Attribute::NonNull;
   case bitc::ATTR_KIND_DEREFERENCEABLE:
     return Attribute::Dereferenceable;
   case bitc::ATTR_KIND_DEREFERENCEABLE_OR_NULL:
     return Attribute::DereferenceableOrNull;
+  case bitc::ATTR_KIND_ALLOC_ALIGN:
+    return Attribute::AllocAlign;
   case bitc::ATTR_KIND_ALLOC_SIZE:
     return Attribute::AllocSize;
   case bitc::ATTR_KIND_NO_RED_ZONE:
     return Attribute::NoRedZone;
   case bitc::ATTR_KIND_NO_RETURN:
     return Attribute::NoReturn;
   case bitc::ATTR_KIND_NOSYNC:
     return Attribute::NoSync;

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

static void writeStringRecord(BitstreamWriter &Stream, unsigned Code,
   // Emit the finished record.
   Stream.EmitRecord(Code, Vals, AbbrevToUse);
 }
 
 static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
   switch (Kind) {
   case Attribute::Alignment:
     return bitc::ATTR_KIND_ALIGNMENT;
+  case Attribute::AllocAlign:
+    return bitc::ATTR_KIND_ALLOC_ALIGN;
   case Attribute::AllocSize:
     return bitc::ATTR_KIND_ALLOC_SIZE;
   case Attribute::AlwaysInline:
     return bitc::ATTR_KIND_ALWAYS_INLINE;
   case Attribute::ArgMemOnly:
     return bitc::ATTR_KIND_ARGMEMONLY;
   case Attribute::Builtin:
     return bitc::ATTR_KIND_BUILTIN;

llvm/lib/Transforms/Utils/CodeExtractor.cpp

Function *CodeExtractor::constructFunction(const ValueSet &inputs,
   for (const auto &Attr : oldFunction->getAttributes().getFnAttrs()) {
     if (Attr.isStringAttribute()) {
       if (Attr.getKindAsString() == "thunk")
         continue;
     } else
       switch (Attr.getKindAsEnum()) {
       // Those attributes cannot be propagated safely. Explicitly list them
       // here so we get a warning if new attributes are added.
+      case Attribute::AllocAlign:
       case Attribute::AllocSize:
       case Attribute::ArgMemOnly:
       case Attribute::Builtin:
       case Attribute::Convergent:
       case Attribute::InaccessibleMemOnly:
       case Attribute::InaccessibleMemOrArgMemOnly:
       case Attribute::JumpTable:
       case Attribute::Naked:

llvm/test/Bitcode/compatibility.ll

 declare void @f.param.stack_align([2 x double] alignstack(16))
 ; CHECK: declare void @f.param.stack_align([2 x double] alignstack(16))
 declare void @f.param.swiftself(i8* swiftself)
 ; CHECK: declare void @f.param.swiftself(i8* swiftself)
 declare void @f.param.swiftasync(i8* swiftasync)
 ; CHECK: declare void @f.param.swiftasync(i8* swiftasync)
 declare void @f.param.swifterror(i8** swifterror)
 ; CHECK: declare void @f.param.swifterror(i8** swifterror)
+declare void @f.param.allocalign(i32 allocalign)
+; CHECK: declare void @f.param.allocalign(i32 allocalign)
 
 ; Functions -- unnamed_addr and local_unnamed_addr
 declare void @f.unnamed_addr() unnamed_addr
 ; CHECK: declare void @f.unnamed_addr() unnamed_addr
 declare void @f.local_unnamed_addr() local_unnamed_addr
 ; CHECK: declare void @f.local_unnamed_addr() local_unnamed_addr
 
 ; Functions -- fn Attrs (Function attributes)