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

[Dan Gohman via Phabricator via llvm-commits]

sunfish added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:111
+        report_fatal_error(
+            "ATOMIC_FENCE is not yet supported in non-emscripten OSes");
+
----------------
aheejin wrote:
> 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
> ```
Ah, you're right, I did a quick test without -mattr=+atomics, which I forget is now the thing which determines whether operations like fence are lowered by LowerAtomic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50277