[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

fhahn added a comment. Someone filed https://github.com/llvm/llvm-project/issues/53540, which covers some of the issues also discussed here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 __

Re: [PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

On Mon, Nov 22, 2021 at 2:28 PM John McCall wrote: > On Mon, Nov 22, 2021 at 1:08 PM David Goldblatt via Phabricator < > revi...@reviews.llvm.org> wrote: > >> and related projects avoid relying on alignment guarantees (e.g. >> libstdc++ at one point considered assuming that 8-byte allocs were 16-

Re: [PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

On Mon, Nov 22, 2021 at 1:08 PM David Goldblatt via Phabricator < revi...@reviews.llvm.org> wrote: > davidtgoldblatt added a comment. > > (For background: I'm a jemalloc maintainer and wrote N2293). > > In D100879#3145361 , @rjmccall > wrote: > > > Platfor

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

davidtgoldblatt added a comment. (For background: I'm a jemalloc maintainer and wrote N2293). In D100879#3145361 , @rjmccall wrote: > Platforms are permitted to make stronger guarantees than the C standard > requires, and it is totally reasonable for co

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

jyknight added a comment. I think this patch was wrong -- we should not be assuming 16-byte alignment for an allocation smaller than 16 bytes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

rjmccall added a comment. Platforms are permitted to make stronger guarantees than the C standard requires, and it is totally reasonable for compilers to assume that `malloc` meets the target platform's documented guarantees. Arguably, libraries should not be replacing `malloc` if they fail to

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 added a comment. Just curious, did you try firefox with malloc? (If you care about the best performance). “ It also mentions that forcing jemalloc to have 16-byte alignment will significantly slow down its performance.” It would be interesting to see real world data, like firefox benc

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

collares added a comment. Reproducing oxalica's comment at https://github.com/NixOS/nixpkgs/pull/146817, to make sure it is not missed: "The divergence here is about how to understand the sentence from C17, 7.22.3. There is already an proposal rephrasing it and making it clear that 8-byte allo

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 added a comment. In D100879#3144751 , @collares wrote: > In https://bugzilla.mozilla.org/show_bug.cgi?id=1741454, oxalica discovered > this breaks jemalloc-using code, since jemalloc does not guarantee 16-byte > alignment. Not sure if jemalloc

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

collares added a comment. In https://bugzilla.mozilla.org/show_bug.cgi?id=1741454, oxalica discovered this breaks jemalloc-using code, since jemalloc does not guarantee 16-byte alignment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 added a comment. In D100879#2714431 , @dmgreen wrote: > Hello. Nice idea. Unfortunately this blocks tail folding, making codesize a > bit bigger: https://godbolt.org/z/rncPvbh8d > I'm guessing it shouldn't? But the attribute isn't handled somewh

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

dmgreen added a comment. Hello. Nice idea. Unfortunately this blocks tail folding, making codesize a bit bigger: https://godbolt.org/z/rncPvbh8d I'm guessing it shouldn't? But the attribute isn't handled somewhere along the way. Any ideas where? Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D100879: [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 rGc2297544c047: [Clang] Propagate guaranteed alignment for malloc and others (authored by xbolva00). Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 ___ cfe-commits mailing list

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 updated this revision to Diff 339912. xbolva00 added a comment. Updated comment for getNewAlign() CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 Files: clang/include/clang/Basic/TargetInfo.h clang/lib/CodeGen/CGCall.cpp clan

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

rjmccall added a comment. Test looks good, thanks. Comment at: clang/lib/CodeGen/CGCall.cpp:2060 +case Builtin::BIstrndup: + RetAttrs.addAlignmentAttr(Context.getTargetInfo().getNewAlign() / +Context.getTargetInfo().getCharWi

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 updated this revision to Diff 339192. xbolva00 added a comment. Fixed small typo in tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 Files: clang/lib/CodeGen/CGCall.cpp clang/test/CodeGen/alloc-fns-alignment.c Index: clang

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 updated this revision to Diff 339191. xbolva00 added a comment. Handle also aligned_alloc and memalign. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 Files: clang/lib/CodeGen/CGCall.cpp clang/test/CodeGen/alloc-fns-alignment.c

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 added a comment. In D100879#2703979 , @rjmccall wrote: > Please addd tests, including tests that we suppress this assumption under > e.g. `-fno-builtin-malloc` Done CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://re

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 updated this revision to Diff 339189. xbolva00 added a comment. Added tests CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 Files: clang/lib/CodeGen/CGCall.cpp clang/test/CodeGen/alloc-fns-alignment.c Index: clang/test/CodeGen/a

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

rjmccall added a comment. Please addd tests, including tests that we suppress this assumption under e.g. `-fno-builtin-malloc` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 ___ cfe-commits mailing

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 added a comment. GCC already knows about this guaranteed alignment, check: https://godbolt.org/z/sxn6K7Yq7 Alignment checks were optimized out. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 _

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2060 +case Builtin::BIstrndup: + RetAttrs.addAlignmentAttr(Context.getTargetInfo().getNewAlign() / +Context.getTargetInfo().getCharWidth()); -

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2060 +case Builtin::BIstrndup: + RetAttrs.addAlignmentAttr(Context.getTargetInfo().getNewAlign() / +Context.getTargetInfo().getCharWidth()); ---

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2059 +case Builtin::BIstrdup: +case Builtin::BIstrndup: + RetAttrs.addAlignmentAttr(Context.getTargetInfo().getNewAlign() / As a followup, I need to teach Clang abou

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 updated this revision to Diff 338950. xbolva00 added a reviewer: jdoerfert. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 Files: clang/lib/CodeGen/CGCall.cpp Index: clang/lib/CodeGen/CGCall.cpp

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 added a comment. If this approach makes sense, I will update / add tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 ___ cfe-commits mailing list

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

xbolva00 created this revision. xbolva00 added reviewers: aaron.ballman, rjmccall. xbolva00 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. LLVM should be smarter about *known* malloc's alignment and this knowledge may enable other optimiz