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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 5 inline comments as done.
aardappel added a comment.

@aheejin yes, this is part of the triple, much like x86 vs x64. That structure was already in place before I started.



================
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>;
 
----------------
aheejin wrote:
> 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`.
I tried to stay close to the original definitions. I do not feel the amount of duplication here is excessive to warrant trying to factor things out into more common definitions, given that trying to abstract things in tablegen leads to less readable results than in a normal programming language.


================
Comment at: llvm/test/MC/WebAssembly/wasm64.s:106
+
+# CHECK:              .globaltype     myglob64, i64
----------------
aheejin wrote:
> 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.
Yes, the value of these CHECKs is low at the moment, but they check a little bit more than just the instruction being accepted, since they convert the instruction back to text. There is nothing in here that likely isn't covered by other tests, but that is hopefully changing soon. 


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

https://reviews.llvm.org/D80769