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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
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)>;
----------------
jgravelle-google wrote:
> 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.
Alas... you'd think so. I certainly thought so. However I tried it, and then I read `include/llvm/Target/TargetSelectionDAG.td` and found that `load` (and the extending loads) are actually PatFrags, while the atomic loads are SDNodes, so they can't be freely mixed in templates like that. I feel like you could maybe make a PatFrag to wrap around the atomic loads and then use the other classes, but I don't know how to do that yet. Perhaps I'll try but I don't know whether it will be worth it or not, especially given we'll have different rules for selecting extending loads anyway (since there are no extending atomic loads), and that there are only 2 types instead of 4. So we'll see when we get more concrete about all of that.


================
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)>;
----------------
jgravelle-google wrote:
> 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.
Ah yeah. I had actually intended to back this part of the CL out once I got deep enough into refactoring the other td file. I'll do that.


================
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)>;
----------------
jgravelle-google wrote:
> What are these non-atomic loads doing here? Also aren't these already represented in WebAssemblyInstrMemory.td?
Yeah this is just a paste that I had before I got down the other rabbit hole. will remove it too.


https://reviews.llvm.org/D37345