[clang][emscripten] Reduce alignof long double from 16 to 8 bytes

[sbc100]

Summary

This means max_align_t is 8 bytes which also sets the alignment
malloc. Since this is technically and ABI breaking change we have
limited to just the emscripten OS target. It is also relatively low
import breakage since it will only effect the alignment of struct that
contai long doubles (extremely rare I imagine).

Emscripten's malloc implementation already use 8 byte alignment
(dlmalloc uses and alignment of 2*sizeof(void*) == 8 rather than
checking max_align_t) so will not be effected by this change. By
bringing the ABI in line with the current malloc code this will fix
several issue we have seen in the wild.

See: https://github.com/emscripten-core/emscripten/pull/14456

[sbc100]

@sunfish what do you think about doing this for all WebAssembly targets?

[rsmith]

clang/lib/Basic/Targets/OSTargets.h

954–956

[sunfish]

Do we still intend to unify Emscripten's ABI with wasm32-unknown-unknown or wasm32-wasi eventually? This is talking a step away from that.

One of the assumptions behind this is that it would be ok for malloc to be 16-byte aligned anyway, because SIMD use cases benefit from being able to call malloc and get a buffer aligned for SIMD. Do we have more information on how much this matters in practice?

Another observation is that part of the reason for long double being 128-bit is to leave room for the possibility of 128-bit floats being added to wasm some day, and on x86-64, aarch64, and powerpc which all have 128-bit long double on Linux, the alignment is 16 bytes.

[kripken]

In D104808#2836942, @sunfish wrote:

Do we still intend to unify Emscripten's ABI with wasm32-unknown-unknown or wasm32-wasi eventually? This is talking a step away from that.

I definitely think we should unify them as much as possible. Ideally we would all make this change.

The motivation for the change is that right now we are losing either some correctness or some performance. Either is a burden, and a possible future float128 in wasm doesn't seem strong enough of a justification to me - should wasm add float128, we can consider a new ABI at that point. Does that sound reasonable?

One of the assumptions behind this is that it would be ok for malloc to be 16-byte aligned anyway, because SIMD use cases benefit from being able to call malloc and get a buffer aligned for SIMD. Do we have more information on how much this matters in practice?

I think the discussions converged on it being ok with the spec, but perhaps a problem in practice, but portable SIMD code seems smart enough to avoid the issue. I don't think I've seen a bug report mentioning SIMD, but I may have missed one.

[sbc100]

In D104808#2836991, @kripken wrote:

In D104808#2836942, @sunfish wrote:

Do we still intend to unify Emscripten's ABI with wasm32-unknown-unknown or wasm32-wasi eventually? This is talking a step away from that.

I definitely think we should unify them as much as possible. Ideally we would all make this change.

The motivation for the change is that right now we are losing either some correctness or some performance. Either is a burden, and a possible future float128 in wasm doesn't seem strong enough of a justification to me - should wasm add float128, we can consider a new ABI at that point. Does that sound reasonable?

One of the assumptions behind this is that it would be ok for malloc to be 16-byte aligned anyway, because SIMD use cases benefit from being able to call malloc and get a buffer aligned for SIMD. Do we have more information on how much this matters in practice?

I think the discussions converged on it being ok with the spec, but perhaps a problem in practice, but portable SIMD code seems smart enough to avoid the issue. I don't think I've seen a bug report mentioning SIMD, but I may have missed one.

Indeed, we have had reports of issues with the under-alignment of values returns from malloc, but I don't think any of them have been related to SIMD, only the expectation that malloc honors max_align_t.

[kripken]

I believe we have all agreed that this is the right path forward, and so this can land?

Doing this change now in emscripten does not prevent us from changing the emscripten ABI again later, as the ABI is unstable (which is why we can make this change itself). We definitely should in the future consider unifying the emscripten triple with WASI, but for now, this change will be a strict improvement in emscripten.

clang/lib/Basic/Targets/OSTargets.h

void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
                     MacroBuilder &Builder) const final {
     WebAssemblyOSTargetInfo<Target>::getOSDefines(Opts, Triple, Builder);
     Builder.defineMacro("__EMSCRIPTEN__");
     if (Opts.POSIXThreads)
       Builder.defineMacro("__EMSCRIPTEN_PTHREADS__");
   }
 
 public:
-  explicit EmscriptenTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
-      : WebAssemblyOSTargetInfo<Target>(Triple, Opts) {}
+  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;
+  }
 };
 
 } // namespace targets
 } // namespace clang
 #endif // LLVM_CLANG_LIB_BASIC_TARGETS_OSTARGETS_H