[PATCH] D71521: Support for library function aligned_alloc

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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.

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



================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:61
+  AllocLike          = MallocOrCallocLike | StrDupLike,
   AnyAlloc           = AllocLike | ReallocLike
 };
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4317
+            return true;
+          }
     } else if (IsCalloc) {
----------------
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.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4300
+                      Attribute::getWithDereferenceableOrNullBytes(
+                          Call.getContext(), Op1C->getZExtValue()));
   } else if (isReallocLikeFn(&Call, TLI) && Op1C) {
----------------
If the alignment is constant we should add a align attribute as well.


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

https://reviews.llvm.org/D71521