[PATCH] D50277: [WebAssembly] Support for atomic fences
Dan Gohman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 28 18:21:54 PDT 2019
sunfish added inline comments.
Comment at: lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:111
+ "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
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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the llvm-commits