[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jdoerfert added a comment. Herald added a subscriber: StephenFan. Herald added a project: All. Are we considering relanding this with smaller alignment for smaller mallocs (and friends)? See: https://github.com/llvm/llvm-project/issues/54747#issuecomment-1088420860 Repository: rG LLVM

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9545976ff160: Revert [Clang] Propagate guaranteed alignment for malloc and others (authored by jyknight). Repository: rG LLVM Github Monorepo

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

MaskRay added a comment. In D118804#3304261 , @urnathan wrote: > While C2X has blessed such smaller alignments, the x86_64 ABI (in > particular), has not. However, using that ABI to justify 'It. Is. 16. > Bytes.', is really an exercise in reality

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

rjmccall added a comment. I'm not sure your new wording is any clearer; a In D118804#3304337 , @urnathan wrote: > In D118804#3304280 , @aaron.ballman > wrote: > >> In D118804#3304261

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

aaron.ballman added a comment. In D118804#3304337 , @urnathan wrote: > In D118804#3304280 , @aaron.ballman > wrote: > >> In D118804#3304261 , @urnathan >> wrote: >> >>>

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. >> I think we need to deal with the reality that there are non-ABI conforming >> [system-dependent] allocators out there, Definitely not a good thing, just ticking bomb. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

urnathan added a comment. In D118804#3304280 , @aaron.ballman wrote: > In D118804#3304261 , @urnathan > wrote: > >> While C2X has blessed such smaller alignments, the x86_64 ABI (in >> particular), has not.

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

aaron.ballman added a comment. In D118804#3304261 , @urnathan wrote: > While C2X has blessed such smaller alignments, the x86_64 ABI (in > particular), has not. However, using that ABI to justify 'It. Is. 16. > Bytes.', is really an exercise in

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

urnathan added a comment. While C2X has blessed such smaller alignments, the x86_64 ABI (in particular), has not. However, using that ABI to justify 'It. Is. 16. Bytes.', is really an exercise in reality denial at this point. just thought I'd make it clear we have conflicting standards and

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

aaron.ballman added a comment. In D118804#3303004 , @jyknight wrote: > In D118804#3302675 , @rsmith wrote: > >> I support this revert. > > Having received enough support, I'll go ahead and commit, and then

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3302675 , @rsmith wrote: > I support this revert. Having received enough support, I'll go ahead and commit, and then propose backport to llvm 14 branch. But -- > - `malloc` always returns storage whose alignment

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

rsmith added a comment. I support this revert. `malloc`, `operator new`, and `operator new[]` (by the latter two I mean the usual global allocation functions, not user-provided ones) follow these rules: - `malloc` always returns storage whose alignment is at least the largest fundamental

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. Also, __builtin_malloc(...) can be used to avoid any alignment assumptions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804 ___

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

Lapenkov added a comment. In D118804#3301753 , @jyknight wrote: > In D118804#3301746 , @Lapenkov > wrote: > >> Is it hard to retrofit this diff into LLVM 13? > > It would be easy to backport this change into

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3301746 , @Lapenkov wrote: > Is it hard to retrofit this diff into LLVM 13? It would be easy to backport this change into LLVM 13, but the problem is that there are no more releases planned on the LLVM 13 branch.

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

Lapenkov added a comment. Frankly, sounds quite inconvenient. This means that the default behavior changes for LLVM 13 and then changes back for LLVM 14. Everyone using a custom allocator with weak alignment will need to know about this caveat and be really wary of this additional flag to

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. llvm 14 - yes llvm 13.0.2 is not planned. If you need a workaround, use -fno-builtin-malloc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

Lapenkov added a comment. Do you think it should be ported to release/13.x too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804 ___ cfe-commits mailing list

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 accepted this revision. xbolva00 added a comment. This revision is now accepted and ready to land. LG. Please wait a day in case any new comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight updated this revision to Diff 406478. jyknight added a comment. Rebase, and preserve (and modify) test-case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804 Files:

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 requested changes to this revision. xbolva00 added inline comments. This revision now requires changes to proceed. Comment at: clang/test/CodeGen/alloc-fns-alignment.c:37 - -void *aligned_alloc_large_constant_test(size_t n) { - return aligned_alloc(4096, n);

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

ychen accepted this revision. ychen added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804 ___ cfe-commits mailing list

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

ychen added a comment. In D118804#3292263 , @jyknight wrote: > In D118804#3292185 , @ychen wrote: > >> I don't see why the patch is wrong... It uses the target/platform-specific >> `NewAlign`. If the platform

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3292185 , @ychen wrote: > I don't see why the patch is wrong... It uses the target/platform-specific > `NewAlign`. If the platform allows customized memory allocation that assumes > weak alignment, it should set the

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. In D118804#3292179 , @MaskRay wrote: > In D118804#3292176 , @xbolva00 > wrote: > Reintroducing an optimization like this with an additional check that the allocation size is

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. Can you also confirm that there is no similar related to op new? Yes, we “can” this assumption by removing it but.. there should be consensus whether just this code is problematic or problem is bigger (as we use NewAlign..) Repository: rG LLVM Github Monorepo

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

ychen added a comment. I don't see why the patch is wrong... It uses the target/platform-specific `NewAlign`. If the platform allows customized memory allocation that assumes weak alignment, it should set the `NewAlign` accordingly, no? Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

MaskRay added a comment. In D118804#3292176 , @xbolva00 wrote: >>> Reintroducing an optimization like this with an additional check that the >>> allocation size is large enough should be valid everywhere. > > Should not be hard, could you do it? Then

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. >> Reintroducing an optimization like this with an additional check that the >> allocation size is large enough should be valid everywhere. Should not be hard, could you do it? Then LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. > For C++, I confess I have some problems interpreting this sentence: > > (https://eel.is/c++draft/cpp.predefined) > >> `__STDCPP_­DEFAULT_­NEW_­ALIGNMENT__`: An integer literal of type >> std::size_t whose value is the alignment guaranteed by a call to operator >>

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. >> mimalloc uses 8-byte alignment for 64-bit Darwin, not 16-byte alignment. Than they are doing something fishy if I understood rjmccall’s comment correctly about darwin. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

MaskRay added a comment. In D118804#3291060 , @xbolva00 wrote: > Or can we bailout just for gnu? and preserve this for darwin? As is, the original patch applies the C++ getNewAlign to C calloc/malloc/strdup/etc. This is outright wrong. For C++, I

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. Or can we bailout just for gnu? and preserve this for darwin? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804 ___ cfe-commits mailing

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a subscriber: collares. jyknight added a comment. In D118804#3290874 , @collares wrote: > might be worth filing a GCC bug as well Yes, a bug report for GCC should be opened as well. @collares do you want to take that? In D118804#3290937

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

MaskRay accepted this revision. MaskRay added a comment. LGTM. This should be ported to release/14.x as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

MaskRay added subscribers: urnathan, MaskRay. MaskRay added a comment. I agree that the original patch (c2297544c04764237cedc523083c7be2fb3833d4 ) should be reverted. The `__STDCPP_DEFAULT_NEW_ALIGNMENT__==16` (on x86-64)

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3290874 , @collares wrote: > It is worth noting that GCC started assuming 16-byte alignment for small > objects in 2016, before N2293 was written and accepted into C2x; see >

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 added a comment. So better to find solution / agreement with gcc devs too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804 ___ cfe-commits mailing

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3290803 , @xbolva00 wrote: > GCC also does same assumptions Looking at GCC: GCC only assumes 4-byte alignment on i386, and 8-byte alignment on x86-64, which is why it hasn't actively broken users, unlike the clang

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight updated this revision to Diff 405315. jyknight added a comment. (fix git mishap: neglected to add a file to original change after conflict resolution) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

collares added a comment. It is worth noting that GCC started assuming 16-byte alignment for small objects in 2016, before N2293 was written and accepted into C2x; see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90569#c8 and https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90569#c9 for more

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

xbolva00 requested changes to this revision. xbolva00 added a comment. This revision now requires changes to proceed. GCC also does same assumptions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight created this revision. jyknight added reviewers: aaron.ballman, rjmccall, jdoerfert, xbolva00. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The above change assumed that malloc (and friends) would always allocate