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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

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.


Ah, interesting point, i have not considered it.
But then we already have that problem in other places,
at least `clang/lib/CodeGen/CGCall.cpp`, `AbstractAssumeAlignedAttrEmitter`.

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

I'm not sure what you mean. Clearly, as per this patch, that is what we do now,
and it fails because there's an artificial (and bogus, too low) upper limit.

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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78810