[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 abandoned this revision. sbc100 added a comment. Yes, on further investigation is seems that there is precedent for having `alignof(max_align_t)` and `__BIGGEST_ALIGNMENT__` be different. See https://github.com/emscripten-core/emscripten/pull/19728 Repository: rG LLVM Github

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

dschuff added a comment. After local discussion, I guess we decided to leave this as-is? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151820/new/ https://reviews.llvm.org/D151820 ___ cfe-commits

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a comment. OK to land this? (With a provision that I will add something to the emscripten changelog?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151820/new/ https://reviews.llvm.org/D151820

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

dschuff added a comment. > Presumably this change of change makes most sense in the emscripten ChangeLog > right? We don't tend to document emscripten-specific changes in the llvm > release notes do we? Yes, I think so. Also I think it's more likely to be seen by users in the emscripten

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a comment. In D151820#4415754 , @dschuff wrote: >> As far as I can tell the only way this change could break XNNpack if >> XNN_ALLOCATION_ALIGNMENT = 8 is wrongly set there... as long as that is the >> correct value for

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

dschuff added a comment. > As far as I can tell the only way this change could break XNNpack if > XNN_ALLOCATION_ALIGNMENT = 8 is wrongly set there... as long as that is the > correct value for XNN_ALLOCATION_ALIGNMENT I don't see how this change could > break it. If XNN_ALLOCATION_ALIGNMENT

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a comment. Can we land this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151820/new/ https://reviews.llvm.org/D151820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a comment. In D151820#4400795 , @tlively wrote: > In D151820#4385512 , @dschuff wrote: > >> I seem to recall that @tlively and I spent a bunch of time with XNNpack >> chasing down some kind of

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

tlively added a comment. In D151820#4385512 , @dschuff wrote: > I seem to recall that @tlively and I spent a bunch of time with XNNpack > chasing down some kind of subtle error that I suspect had to do with > alignment, but maybe he remembers that

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a subscriber: ngzhian. sbc100 added a comment. In D151820#4385568 , @sbc100 wrote: > In D151820#4385536 , @dschuff wrote: > >>> I don't think it will since `__BIGGEST_ALIGNMENT__ >= >>>

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a comment. In D151820#4385536 , @dschuff wrote: >> I don't think it will since `__BIGGEST_ALIGNMENT__ >= >> XNN_ALLOCATION_ALIGNMENT` will remain true after this change.. so this >> change should have no effect on that code. > > I meant

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

dschuff added a comment. > I don't think it will since `__BIGGEST_ALIGNMENT__ >= > XNN_ALLOCATION_ALIGNMENT` will remain true after this change.. so this change > should have no effect on that code. I meant that when `__BIGGEST_ALIGNMENT__ >= XNN_ALLOCATION_ALIGNMENT` (which was true before

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a comment. In D151820#4385512 , @dschuff wrote: >> I don't think it will change anything in that code since >> `__BIGGEST_ALIGNMENT__ >= XNN_ALLOCATION_ALIGNMENT` will still hold true >> both before and after this change

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

dschuff added a subscriber: tlively. dschuff added a comment. > I don't think it will change anything in that code since > `__BIGGEST_ALIGNMENT__ >= XNN_ALLOCATION_ALIGNMENT` will still hold true both > before and after this change (XNN_ALLOCATION_ALIGNMENT == 4 on wasm) Right, that check

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a comment. In D151820#4385393 , @dschuff wrote: > I guess this is basically the C version of max_align_t so it should match. Yes, it should match. Having `__BIGGEST_ALIGNMENT__` be 16 for emscripten doesn't make any sense right now. >

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 added a comment. In D151820#4385393 , @dschuff wrote: > I guess this is basically the C version of max_align_t so it should match. > but... this still has the potential to break things. True, but I think it's not as likely the break things as

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

dschuff added a comment. I guess this is basically the C version of max_align_t so it should match. but... this still has the potential to break things. e.g. it will change the allocation in https://github.com/google/XNNPACK/blob/master/src/xnnpack/allocator.h#L66 ISTR that was one of the

[PATCH] D151820: [clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

sbc100 created this revision. Herald added subscribers: pmatos, wingo, sunfish, jgravelle-google, dschuff. Herald added a project: All. sbc100 requested review of this revision. Herald added subscribers: cfe-commits, aheejin. Herald added a project: clang. Follow up to