[PATCH] D71521: Support for library function aligned_alloc

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked 3 inline comments as done.
bondhugula added a comment.

In D71521#1785281 <https://reviews.llvm.org/D71521#1785281>, @jdoerfert wrote:

> This test contains various changes in different parts of the code plus unrelated fixes (in comments, spelling, formatting, ...).
>
> I think it is beneficial to have a patch that recognizes aligned_alloc, which should be testable through existing calls to isAllocLike, and one that teaches the transformations about it. All the unrelated NFC changes can go in as NFC patches before hand and without pre-review. Generally speaking, smaller patches are good. More practically, you now need reviewers that accept changes in various places. Granted, the changes are not complex in nature, but the general logic still applies.


I'll defer it to you and other reviewers on how you'd like this to be split once the changes are finalized.

> We should also determine if aligned_alloc is the only one we want to support in this family or not. I mean, there is stuff like posix_memalign, memalign, pvalloc, ...

posix_memalign is unfortunately very different - it doesn't return the pointer, but instead takes the address of the pointer as an argument. It's going to be quite non-trivial to support that AFAICS albeit it is similar to aligned_alloc on other aspects. valloc, pvalloc, memalign are all obsolete - I think aligned_alloc will by itself really addresses a lot of use cases. On a side note, there is little that posix_memalign provides that aligned_alloc doesn't because if you want an alignment that malloc doesn't already provide you (> 16 bytes), you will basically have large "elements" (>= 256-bit vectors typically) and as such your allocation size will already be a multiple of that alignment, which is a constraint with aligned_alloc but not with posix_memalign.



================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:61
+  AllocLike          = MallocOrCallocLike | StrDupLike,
   AnyAlloc           = AllocLike | ReallocLike
 };
----------------
jdoerfert wrote:
> I'm not sure we want to pretend aligned_alloc is malloc or calloc like. It might not matter at the end of the day but for example they (can) behave differently when it comes to freeing memory. Would it be a problem to put it under `AllocLike` only?
> I'm not sure we want to pretend aligned_alloc is malloc or calloc like. It might not matter at the end of the day but for example they (can) behave differently when it comes to freeing memory. Would it be a problem to put it under AllocLike only?

I actually thought about this, and looking at how MallocOrCallocLike is used, aligned_alloc behaves the same way as far as this property goes. The freeing of memory works the same way, and the fact that malloc and calloc already have size arguments appearing in different positions/ways makes aligned_alloc fit with the common property implied by MallocOrCallocLike. It's nice thus that all the places that were already using MallocOrCallocLike now get the benefit of being able to deal with aligned_alloc as well transparently.




================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4317
+            return true;
+          }
     } else if (IsCalloc) {
----------------
jdoerfert wrote:
> bondhugula wrote:
> > bondhugula wrote:
> > > jdoerfert wrote:
> > > > Shouldn't the `isa<ConstantInt>(I.getOperand(0)` be inside the `if (IsAlignedAllocLike)`?
> > > > 
> > > > Do we really want to skip on heap 2 stack if the alignment is dynamic?
> > > The 'alloca' instruction takes a constant alignment (both from the langref and the AllocaInst ctors). Also, a dynamic alignment would be tantamount to a dynamic sized allocation, which we aren't converting to stack allocations. (The padding needed for a dynamic alignment could effectively lead to an allocation of at least that size in the worst case.)
> > > Shouldn't the `isa<ConstantInt>(I.getOperand(0)` be inside the `if (IsAlignedAllocLike)`?
> > 
> > It'd be the same that way, but you'll need an extra line and indent for the rest. I can drop the comment under.
> > It'd be the same that way, but you'll need an extra line and indent for the rest. I can drop the comment under.
> 
> I'm aware it is the same but it would be the same style as the surrounding code. Having an extra lines is not a problem, having these lines indented one more level is neither. I don't know what you mean by "drop the comment under".
> 
> > no dynamic alignment
> 
> OK.
The code comment right under saying "alignment and size being constant" was sort of misplaced - I meant dropping it given where I had the extra guard (&&). Anyway, I'm fine with moving the guard inside.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4300
+                      Attribute::getWithDereferenceableOrNullBytes(
+                          Call.getContext(), Op1C->getZExtValue()));
   } else if (isReallocLikeFn(&Call, TLI) && Op1C) {
----------------
jdoerfert wrote:
> If the alignment is constant we should add a align attribute as well.
Actually, it need not be! :-) The alignment with the aligned_alloc call can be dynamic - it's just that we convert it to a stack allocation only when it's constant.


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

https://reviews.llvm.org/D71521