[clang][WebAssembly] Fix __BIGGEST_ALIGNMENT__ under emscripten

[sbc100]

Summary

Follow up to https://reviews.llvm.org/D104808 and
https://reviews.llvm.org/D105749. The only place that seems to be used
is when defining __BIGGEST_ALIGNMENT__ and when choosing the alignment
for the alloca builtin. It seems that both of these should match the
alignof(max_align_t) which, for emscripten, is currently 8 bytes.

[dschuff]

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 projects that had an issue with this the first time around?

[sbc100]

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 that change the max_align_t.. which is more commonly used.

e.g. it will change the allocation in https://github.com/google/XNNPACK/blob/master/src/xnnpack/allocator.h#L66

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)

ISTR that was one of the projects that had an issue with this the first time around?

[sbc100]

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.

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 projects that had an issue with this the first time around?

[dschuff]

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 causes XNN_ALLOCATION_ALIGNMENT to be ignored in favor of using clang's _builtin_alloca() which will be changed by this CL.
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 better than I do.

[sbc100]

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 (XNN_ALLOCATION_ALIGNMENT == 4 on wasm)

Right, that check causes XNN_ALLOCATION_ALIGNMENT to be ignored in favor of using clang's _builtin_alloca() which will be changed by this CL.

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.

[dschuff]

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 and will remain true), then XNNPack uses __builtin_alloca() as the implementation of XNN_SIMD_ALLOCA (which presumably is for allocating SIMD values). This change will reduce the alignment used by __builtin_alloca() from 16 to 8, such that (I think) it is no longer suitable for SIMD values.

Maybe this is a bug in XNNPack (they should maybe be using XNN_ALLOCATION_ALIGNMENT with a value suitable for SIMD?) but given that BIGGEST_ALIGNMENT and alloca seem to be intended for any base type (including SIMD) it wouldn't be surprising if someone else were depending on this too.

which... maybe this is just re-litigating the previous discussion, I don't know. I wonder at what point our ABI should be treating SIMD values as "normal" rather than rare.

[sbc100]

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 that when __BIGGEST_ALIGNMENT__ >= XNN_ALLOCATION_ALIGNMENT (which was true before and will remain true), then XNNPack uses __builtin_alloca() as the implementation of XNN_SIMD_ALLOCA (which presumably is for allocating SIMD values). This change will reduce the alignment used by __builtin_alloca() from 16 to 8, such that (I think) it is no longer suitable for SIMD values.

Maybe this is a bug in XNNPack (they should maybe be using XNN_ALLOCATION_ALIGNMENT with a value suitable for SIMD?) but given that BIGGEST_ALIGNMENT and alloca seem to be intended for any base type (including SIMD) it wouldn't be surprising if someone else were depending on this too.

XNN_ALLOCATION_ALIGNMENT is 8 under webassembly, which apparently the alignment than xnnpack wants for webassemebly. Using alloca for this is find both before and after this change since both 8 and 18 as fit this requirement.

which... maybe this is just re-litigating the previous discussion, I don't know. I wonder at what point our ABI should be treating SIMD values as "normal" rather than rare.

[sbc100]

In D151820#4385568, @sbc100 wrote:

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 that when __BIGGEST_ALIGNMENT__ >= XNN_ALLOCATION_ALIGNMENT (which was true before and will remain true), then XNNPack uses __builtin_alloca() as the implementation of XNN_SIMD_ALLOCA (which presumably is for allocating SIMD values). This change will reduce the alignment used by __builtin_alloca() from 16 to 8, such that (I think) it is no longer suitable for SIMD values.

Maybe this is a bug in XNNPack (they should maybe be using XNN_ALLOCATION_ALIGNMENT with a value suitable for SIMD?) but given that BIGGEST_ALIGNMENT and alloca seem to be intended for any base type (including SIMD) it wouldn't be surprising if someone else were depending on this too.

XNN_ALLOCATION_ALIGNMENT is 8 under webassembly, which apparently the alignment than xnnpack wants for webassemebly. Using alloca for this is find both before and after this change since both 8 and 18 as fit this requirement.

which... maybe this is just re-litigating the previous discussion, I don't know. I wonder at what point our ABI should be treating SIMD values as "normal" rather than rare.

If xnnpack wanted more than 8 byte alignment it should surely set XNN_ALLOCATION_ALIGNMENT to greater than 8?

Adding @tlively and @ngzhian in case that have some more background on this..

[tlively]

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 better than I do.

Sorry, I have no recollection of this at all 😬

[sbc100]

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 subtle error that I suspect had to do with alignment, but maybe he remembers that better than I do.

Sorry, I have no recollection of this at all 😬

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 is set wrongly this change might expose that bug.. but it seems correct to me.

[sbc100]

Can we land this?

[dschuff]

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 is set wrongly this change might expose that bug.. but it seems correct to me.

yeah, that's actually what my concern is. IIUC as written the code is asking for 8, but it's being masked by our value of BIGGEST_ALIGNMENT.

I suppose we should land this since I think we do want to have it match max_align_t. But it does make me wonder (again) whether our choice of ABI is correct here.
Can you also put something in the emscripten release notes about this?

[sbc100]

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 XNN_ALLOCATION_ALIGNMENT I don't see how this change could break it. If XNN_ALLOCATION_ALIGNMENT is set wrongly this change might expose that bug.. but it seems correct to me.

yeah, that's actually what my concern is. IIUC as written the code is asking for 8, but it's being masked by our value of BIGGEST_ALIGNMENT.

I suppose we should land this since I think we do want to have it match max_align_t. But it does make me wonder (again) whether our choice of ABI is correct here.
Can you also put something in the emscripten release notes about this?

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?

[dschuff]

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 changelog

[sbc100]

OK to land this? (With a provision that I will add something to the emscripten changelog?)

[dschuff]

After local discussion, I guess we decided to leave this as-is?

[sbc100]

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

clang/lib/Basic/Targets/OSTargets.h

explicit EmscriptenTargetInfo(const llvm::Triple &Triple,
                                 const TargetOptions &Opts)
       : WebAssemblyOSTargetInfo<Target>(Triple, Opts) {
     // Keeping the alignment of long double to 8 bytes even though its size is
     // 16 bytes allows emscripten to have an 8-byte-aligned max_align_t which
     // in turn gives is a 8-byte aligned malloc.
     // Emscripten's ABI is unstable and we may change this back to 128 to match
     // the WebAssembly default in the future.
     this->LongDoubleAlign = 64;
+    this->SuitableAlign = 64;
   }
 };
 
 // OHOS target
 template <typename Target>
 class LLVM_LIBRARY_VISIBILITY OHOSTargetInfo : public OSTargetInfo<Target> {
 protected:
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,

clang/test/Preprocessor/init.c

 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=xcore-none-none < /dev/null | FileCheck -match-full-lines -check-prefix XCORE %s
 // XCORE:#define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
 // XCORE:#define __LITTLE_ENDIAN__ 1
 // XCORE:#define __XS1B__ 1
 // XCORE:#define __xcore__ 1
 //
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm32-unknown-unknown \
 // RUN:   < /dev/null \
-// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY32 %s
+// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY32,WEBASSEMBLY-DEFAULT %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm64-unknown-unknown \
 // RUN:   < /dev/null \
-// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY64 %s
+// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY64,WEBASSEMBLY-DEFAULT %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm32-emscripten \
 // RUN:   < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY32,EMSCRIPTEN %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm32-emscripten -pthread -target-feature +atomics -target-feature +bulk-memory \
 // RUN:   < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY32,EMSCRIPTEN,EMSCRIPTEN-THREADS %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm64-emscripten \
 // RUN:   < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY64,EMSCRIPTEN %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm32-wasi \
 // RUN:   < /dev/null \
-// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY32,WEBASSEMBLY-WASI %s
+// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY32,WEBASSEMBLY-DEFAULT,WEBASSEMBLY-WASI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm64-wasi \
 // RUN:   < /dev/null \
-// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY64,WEBASSEMBLY-WASI %s
+// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY64,WEBASSEMBLY-DEFAULT,WEBASSEMBLY-WASI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm32-unknown-unknown -x c++ \
 // RUN:   < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY-CXX %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=wasm32-unknown-unknown -x c++ -pthread -target-feature +atomics -target-feature +bulk-memory \
 // RUN:   < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY-CXX-ATOMICS %s
 //
 // WEBASSEMBLY32:#define _ILP32 1
 // WEBASSEMBLY32-NOT:#define _LP64
 // WEBASSEMBLY64-NOT:#define _ILP32
 // WEBASSEMBLY64:#define _LP64 1
 // EMSCRIPTEN-THREADS:#define _REENTRANT 1
 // WEBASSEMBLY-NEXT:#define __ATOMIC_ACQUIRE 2
 // WEBASSEMBLY-NEXT:#define __ATOMIC_ACQ_REL 4
 // WEBASSEMBLY-NEXT:#define __ATOMIC_CONSUME 1
 // WEBASSEMBLY-NEXT:#define __ATOMIC_RELAXED 0
 // WEBASSEMBLY-NEXT:#define __ATOMIC_RELEASE 3
 // WEBASSEMBLY-NEXT:#define __ATOMIC_SEQ_CST 5
-// WEBASSEMBLY-NEXT:#define __BIGGEST_ALIGNMENT__ 16
+// EMSCRIPTEN-NEXT:#define __BIGGEST_ALIGNMENT__ 8
+// WEBASSEMBLY-DEFAULT-NEXT:#define __BIGGEST_ALIGNMENT__ 16
 // WEBASSEMBLY-NEXT:#define __BITINT_MAXWIDTH__ 128
 // WEBASSEMBLY-NEXT:#define __BOOL_WIDTH__ 8
 // WEBASSEMBLY-NEXT:#define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
 // WEBASSEMBLY-NEXT:#define __CHAR16_TYPE__ unsigned short
 // WEBASSEMBLY-NEXT:#define __CHAR32_TYPE__ unsigned int
 // WEBASSEMBLY-NEXT:#define __CHAR_BIT__ 8
 // WEBASSEMBLY-NOT:#define __CHAR_UNSIGNED__
 // WEBASSEMBLY-NEXT:#define __CLANG_ATOMIC_BOOL_LOCK_FREE 2