[PATCH] D79542: [WebAssembly] Disallow 'shared-mem' rather than 'atomics'

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 2 inline comments as done.
tlively added a comment.

In D79542#2024946 <https://reviews.llvm.org/D79542#2024946>, @aheejin wrote:

> > This was previously done by disallowing the 'atomics' feature, but that could lead to an awkward situation in which an object was marked as disallowing atomics when it in fact used atomics itself. Once atomic operations are allowed in modules with unshared memory, this would result in modules containing atomics but not having the 'atomics' feature in their target-features sections.
>
> Is this possible now? When we disallow atomics now, haven't we already stripped all atomic instructions by then, or we didn't have atomics in the first place? How can a module contain atomics and disallow it at the same time?


Consider compiling a module that contains TLS and has 'atomics' enabled but not 'bulk-memory'. The module's TLS and atomic operations will be lowered away because TLS requires bulk-memory. Note that intrinsics like @llvm.wasm.notify and friends are not currently lowered away, which is how you can end up with a module that has its atomics stripped but still contains atomics. But we could fix that by lowering away the notify and wait intrinsics, and then you'd be right that this is impossible.

I suppose the simpler and more important reason for this change is that it is perfectly safe to link an object with atomics with an object with stripped atomics as long as the resulting module does not use a shared memory, but this is disallowed when the stripped object has '-atomics'. I'll update the message to explain that instead.



================
Comment at: lld/test/wasm/shared-memory-no-atomics.yaml:60
 
-# SHARED: 'atomics' feature is disallowed by {{.*}}shared-memory-no-atomics.yaml.tmp1.o, so --shared-memory must not be used{{$}}
+# SHARED: shared memory is disallowed by {{.*}}shared-memory-no-atomics.yaml.tmp1.o because it was not compiled with 'atomics' or 'bulk-memory' features, so --shared-memory must not be used.
----------------
sbc100 wrote:
> This sentence sounds a little redundant to me?  The part about shared memory not being allowed.  Maybe its just me ..
Sure, I'll remove everything after the comma and change the beginning to use the literal flag name.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:265
+  // This pseudo-feature tells the linker whether shared memory would be safe
+  EmitFeature("shared-mem");
 
----------------
sbc100 wrote:
> Why not add this to WebAssemblyFeatureKV?
WebAssemblyFeatureKV is generated from the TableGen definitions of the WebAssembly subtarget features. I didn't want to go so far as to add a real feature for this, though, since users should not be manually enabling or disabling it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79542/new/

https://reviews.llvm.org/D79542