[PATCH] D80769: [WebAssembly] Adding 64-bit versions of all load & store ops.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 4 inline comments as done.
aardappel added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrMemory.td:62
 // Basic load.
 // FIXME: When we can break syntax compatibility, reorder the fields in the
 // asmstrings to match the binary encoding.
----------------
dschuff wrote:
> side note: since we are about to start depending even more on the asm syntax, and having users try to use it, it's probably getting to now-or-never on this fix.
Is this about swapping offset & align? I could do that in a next commit, though personally this doesn't sound important to me. @sbc100 opinion?


================
Comment at: llvm/test/CodeGen/WebAssembly/cpus.ll:4
 ; RUN: llc < %s -asm-verbose=false -mtriple=wasm32-unknown-unknown -mcpu=mvp 2>&1 | FileCheck %s
-; RUN: not --crash llc < %s -asm-verbose=false -mtriple=wasm64-unknown-unknown-wasm -mcpu=mvp 2>&1 | FileCheck %s --check-prefix=WASM64
+; RUN: llc < %s -asm-verbose=false -mtriple=wasm64-unknown-unknown-wasm -mcpu=mvp 2>&1 | FileCheck %s
 ; RUN: llc < %s -asm-verbose=false -mtriple=wasm32-unknown-unknown -mcpu=generic 2>&1 | FileCheck %s
----------------
dschuff wrote:
> probably not to fix in this CL, but triple=wasm64 and -mcpu=mvp probably shouldn't be allowed.
I was actually thinking about that, but looking at the code that deals with the cpu flags (in WebAssembly.cpp) and couldn't see a good place to error out on this. Maybe in `setABI` ?
Also, what should the default cpu be if someone selects `wasm64` ?


================
Comment at: llvm/test/CodeGen/WebAssembly/load-ext.ll:6
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
----------------
dschuff wrote:
> should the datalayout be removed here too?
> I think it does make sense for many of these simple tests to use the target-defined datalayout (i.e. use the same IR for both 32 and 64) but we should be a bit careful with that, given that the frontend is going to generate actually-different IR for some cases.
Yes, forgot to delete it here. I assumed it was better to rely on whatever is default for wasm32/64, but not familiar with under what conditions this could produce values other than the ones in `datalayout`.


================
Comment at: llvm/test/MC/WebAssembly/wasm64.s:2
+# RUN: llvm-mc -triple=wasm64-unknown-unknown -mattr=+atomics,+unimplemented-simd128,+nontrapping-fptoint,+exception-handling < %s | FileCheck %s
+# Check that it converts to .o without errors, but don't check any output:
+# RUN: llvm-mc -triple=wasm64-unknown-unknown -filetype=obj -mattr=+atomics,+unimplemented-simd128,+nontrapping-fptoint,+exception-handling -o %t.o < %s
----------------
dschuff wrote:
> Are we able to check the encoding of the instructions yet? See e.g. https://github.com/llvm/llvm-project/blob/master/llvm/test/MC/WebAssembly/atomics-encodings.s
Yes, we should be able to add that to this test. Not sure what the point is though, since all load/store ops have exactly the same encoding as in wasm32. 

Frankly, this test doesn't test anything, since the only thing that changes is that load/store now accept 64-bit operands. We need to test that those are being emitted by LLVM (which we don't test here), and that that combination is accepted by verifiers (which we also don't test here, the assembler doesn't verify types, and happily constructs illegal instruction sequences).

What is new here (and is currently commented out pending new reloc types) is `i64.const label`, but we can't check the encoding of that, since at the .o level it will just be all zeroes.


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

https://reviews.llvm.org/D80769