[PATCH] D117921: Attributes: add a new allocalign attribute

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

In D117921#3294602 <https://reviews.llvm.org/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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117921/new/

https://reviews.llvm.org/D117921