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

[Derek Schuff via Phabricator via llvm-commits]

dschuff 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.
----------------
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.


================
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
----------------
probably not to fix in this CL, but triple=wasm64 and -mcpu=mvp probably shouldn't be allowed.


================
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"
----------------
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.


================
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
----------------
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


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

https://reviews.llvm.org/D80769