[PATCH] D37345: [WebAssembly] Refactor load ISel tablegen patterns into classes

[Jacob Gravelle via Phabricator via llvm-commits]

jgravelle-google added a comment.

As an overall comment, this is awesome. Big fan of reducing syntactic noise.



================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrAtomics.td:27
 let Predicates = [HasAtomics] in {
-def : Pat<(i32 (atomic_load I32:$addr)), (ATOMIC_LOAD_I32 0, 0, $addr)>;
+class ALoadPatNoOffset<ValueType ty, SDNode node, I inst> :
+  Pat<(ty (node I32:$addr)), (inst 0, 0, $addr)>;
----------------
Is this actually different from `LoadPatNoOffset`? I'd think you could pass `load` or `atomic_load` as the `node` parameter and it should work either way.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrAtomics.td:34
+// Select loads with a constant offset.
+def : Pat<(i32 (atomic_load (regPlusImm I32:$addr, imm:$off))),
+          (ATOMIC_LOAD_I32 0, imm:$off, $addr)>;
----------------
These look like they could use an `ALoadPatImmOff`

Also technically this is defining additional patterns, and isn't covered by the commit message. I'm ok with that though.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrAtomics.td:48
+          (ATOMIC_LOAD_I64 0, tglobaladdr:$off, $addr)>;
+def : Pat<(i32 (load (add I32:$addr, (WebAssemblywrapper texternalsym:$off)))),
+          (LOAD_I32 0, texternalsym:$off, $addr)>;
----------------
What are these non-atomic loads doing here? Also aren't these already represented in WebAssemblyInstrMemory.td?


https://reviews.llvm.org/D37345