[PATCH] D71521: Support for library function aligned_alloc

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

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

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


Your earlier comment suggested splitting the heap to stack stuff from the rest. But what you suggest above appears to be different from that. For eg., InstCombine needs an isAlignedAllocLikeFn: would that go into the patch that "recognizes aligned_alloc" or the one that makes the transformations aware? If it's the former, I don't see why the heap to stack allocation should be split. If it's the latter, then there is really little that would go into the first patch that recognizes aligned_alloc and it would be logically incomplete. aligned_alloc is recognized via all of: MallocOrCallocLike, AlignedAllocLike, AllocLike, and AnyAlloc, and as such transformations are impacted the moment it's added to MemoryBuiltins. Separating the two is harder for reviewing FWIW. I'm happy to split it the way the reviewers prefer but frankly I don't think there is value in splitting this change list.


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

https://reviews.llvm.org/D71521