[PATCH] D71521: Support for library function aligned_alloc

Sun Dec 15 21:24:38 PST 2019

jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

In D71521#1785299 <https://reviews.llvm.org/D71521#1785299>, @bondhugula wrote:

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

Generally speaking again, if a reviewer ask to split a patch, it most often means they will not review the one they deem to big.

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

We could still encode the allocation nature and/or the alignment it if we deem it important enough. Though we will either need to introduce new attributes/logic, a load, or a wrapper. The TODO in the code is probably fine for this case.

> valloc, pvalloc, memalign are all obsolete

I don't know what difference this really makes. FWIW, We recognize memalign/valloc already in some places but we don't treat them as allocators for some reason.

> I think aligned_alloc will by itself really addresses a lot of use cases.

Sure. Getting all of them, especially if there is almost no extra cost, is still worth it. Not everybody writes C11 and/or updated their code.

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

https://reviews.llvm.org/D71521