[PATCH] D78810: [InstCombine] Check max alignment before adding attr on aligned_alloc

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 17 14:35:12 PDT 2020


jdoerfert added a comment.

In D78810#2098816 <https://reviews.llvm.org/D78810#2098816>, @clin1 wrote:

> FWIW, dropping the attribute LGTM. The heap/stack promotion is still a work in progress, is that correct? At least, I don't see it happening in trunk.


You'll get basic support if you enable the Attributor (`-attributor-enable=cgscc`).

In D78810#2002021 <https://reviews.llvm.org/D78810#2002021>, @bondhugula wrote:

> In D78810#2001925 <https://reviews.llvm.org/D78810#2001925>, @lebedev.ri wrote:
>
> > Thank you for looking into this.
> >  This should instead clamp to the maximal alignment. (the 'is power of 2' check should remain)
>
>
> Clamping would be incorrect here since the allocation risks not providing the desired alignment if it gets promoted to alloca. aligned_alloc doesn't appear to have an upper bound on ailgnment (any power of 2 size_t value is fine I think), while the 1 << 29 limit is for LLVM's alloca among others. I think the right/better fix for this could be to just add the attribute to aligned_alloc as long as it's a power of two, but not promote to alloca when it's larger than 1 << 29.  But then I'm not sure if that would cause other undesired behavior in LLVM with an alignment attribute more than 1 << 29. If it did, then not adding the attribute at all for larger than MaximumAlignment appears to be the safe/right thing.


While I agree this seems to be a safe fix, I fail to see how clamping or promotion to alloca would be problematic: 
First, the attribute:
 `align(X)` in IR means the alignment is at least `X`. We can always use a factor of the real alignment as `X`, which is what clamping would result in, right? So from the IR semantics standpoint `align(1<<29)` would be fine if the argument to `aligned_alloc` is a constant power of two bigger than `1<<29`.
Second, the promotion:
 If we have a call to `XXXalloc` that we want to replace with a stack allocation we need to ensure the guaranteed alignment is not decreased. Assuming we don't know the guaranteed alignment of the call, we are out of luck wrt. promotion. Assuming the alignment is too much for the stack, we have to give up as well. Assuming we know the guaranteed alignment and it is a constant small enough for us to put on an `alloca` instruction, we can go ahead. What I try to say is: The annotation of a minimal alignment (the IR attribute `align`) is irrelevant when it comes to promotion to alloca.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78810





More information about the llvm-commits mailing list