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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

In D78810#2002195 <https://reviews.llvm.org/D78810#2002195>, @lebedev.ri wrote:

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


No, this patch is not adding the alignment attribute at all. So, the clang assertion (in the bug report) goes away. The heap to stack promotion will pull the alignment attribute from the call operand and that would have to be guarded so that the promotion doesn't happen for alignments larger than 1 << 29 (separate bug) because that's what alloca is able to support.

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