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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Question: Is this feature gonna be handled by `-mtriple` and not `-mattrs` like other features?



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrAtomics.td:128
+def : WaitPatNoOffset<i32, int_wasm_atomic_wait_i32, I64, ATOMIC_WAIT_I32_A64>;
+def : WaitPatNoOffset<i64, int_wasm_atomic_wait_i64, I64, ATOMIC_WAIT_I64_A64>;
 
----------------
Patterns and instruction definitions in WebAssemblyInstrMemory.td are defined as multiclasses to group A32 and A64 versions. I think it's better to be consistent here too..? The same for all patterns and defs for `atomic.wait`, `atomic.notify` and `AStoreOffset`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrMemory.td:44
+multiclass WebAssemblyLoad<WebAssemblyRegClass rc, string Name, int Opcode,
+                           list<Predicate> reqs> {
+  let mayLoad = 1, UseNamedOperandTable = 1 in {
----------------
Nit: Can we set do `reqs = []` as a default argument so we don't need `[]` in many cases below?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrMemory.td:72
+  def : Pat<(ty (kind I32:$addr)), (!cast<NI>(inst # "_A32") 0, 0, I32:$addr)>;
+  def : Pat<(ty (kind I64:$addr)), (!cast<NI>(inst # "_A64") 0, 0, I64:$addr)>;
+}
----------------
`Requires` only affects patterns and not instruction definitions. Not sure if it is Tablegen bug... So I think it's better to have `Requires<HasAddr64>` in patterns, here and others, like
```
multiclass LoadPatNoOffset<ValueType ty, PatFrag kind, string inst> {
  def : Pat<(ty (kind I32:$addr)), (!cast<NI>(inst # "_A32") 0, 0, I32:$addr)>;
  def : Pat<(ty (kind I64:$addr)), (!cast<NI>(inst # "_A64") 0, 0, I64:$addr)>, Requires<[HasAddr64]>;
}
```

The same for other patterns in this file, atomics and SIMD.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrMemory.td:247
+                  !strconcat(Name, "\t${off}(${addr})${p2align}, $val"),
+                  !strconcat(Name, "\t${off}${p2align}"), Opcode>;
 }
----------------
Don't we need `reqs` parameter and `Requires<!listconcat(reqs, [HasAddr64])>` here as in `WebAssemblyLoad` here?

But actually the funny thing is `Requires` doesn't work in instruction definitions.. Not sure if it's a Tablegen bug. We have them on definitions anyway, and I think it's ok, and and if we have them it's better to be consistent.


================
Comment at: llvm/test/MC/WebAssembly/wasm64.s:106
+
+# CHECK:              .globaltype     myglob64, i64
----------------
If the only thing this file tests currently is whether LLVM accepts this file without an error, do we need to have these `CHECK` lines, which are the same as the original program? Roundtripping capability is tested in other files already. I might be mistaken, so please let me know if so.


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

https://reviews.llvm.org/D80769