[PATCH] D50277: [WebAssembly] Support for atomic fences

[Heejin Ahn via Phabricator via llvm-commits]

aheejin marked an inline comment as done.
aheejin added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:111
+        report_fatal_error(
+            "ATOMIC_FENCE is not yet supported in non-emscripten OSes");
+
----------------
sunfish wrote:
> aheejin wrote:
> > sunfish wrote:
> > > My understanding of https://github.com/WebAssembly/tool-conventions/issues/59 is that we can lower fence to zero instructions here, rather than aborting.
> > Sorry I didn't see this before committing. This is just to ensure the current crashing behavior for non-emscripten OSes. This does not change anything for them. Didn't we decide that we should commit this CL only for emscripten triple and wait for other OSes until we make a final decision on fences?
> The behavior before this patch is to emit zero instructions, and not crash. So this patch does change the behavior. I'm asking to restore this previous behavior for non-Emscripten OS's.
Really? Without this patch, it crashes for me. Of course, only when we have `-mattr=+atomics`; if we don't have atomics enabled all atomic instructions become normal instructions and fences become nothing for all triples. This behavior is still the same.
This is when I enabled `-mattr=+atomics` with wasi triple, before this patch landed:
```
$ llc atomic-fence.ll -mtriple=wasm32-unknown-wasi -mattr=+atomics,+sign-ext
LLVM ERROR: Cannot select: t3: ch = AtomicFence t0, Constant:i32<7>, Constant:i32<1>
  t1: i32 = Constant<7>
  t2: i32 = Constant<1>
In function: multithread_fence
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50277