[PATCH] D37445: [globalisel][tablegen] Map ld and st to G_LOAD and G_STORE. NFC

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/Target/GlobalISel/SelectionDAGCompat.td:83
+// isSignExtLoad require that this is not a perfect 1:1 mapping since a
+// sign-extending load is (G_SEXT (G_LOAD x)) in GlobalISel.
+def : GINodeEquiv<G_LOAD, ld> { let CheckMMOIsNonAtomic = 1; }
----------------
qcolombet wrote:
> Although I get the comment, I don't see how this affect the following definition.
> I.e., what in the following definition changed compared to the surrounding ones to take this into account?
I think I've missed a 'For example' in the comment but it would be clearer to explain the Atomic bit as well. I'll update the comment.


================
Comment at: include/llvm/Target/GlobalISel/SelectionDAGCompat.td:88
+// isTruncStore require that this is not a perfect 1:1 mapping since a
+// truncating store is (G_TRUNCATE (G_LOAD x)) in GlobalISel.
+def : GINodeEquiv<G_STORE, st> { let CheckMMOIsNonAtomic = 1; }
----------------
qcolombet wrote:
> Typo (G_STORE (G_TRUNCATE x))
Thanks for spotting that. I'm not sure what I was thinking there.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1996
   /// This is defined using 'GINodeEquiv' in the target description.
-  DenseMap<Record *, const CodeGenInstruction *> NodeEquivs;
+  DenseMap<Record *, Record *> NodeEquivs;
 
----------------
qcolombet wrote:
> I don't get why this change is required yet. G_LOAD and G_STORE records are still CodeGenInstruction, aren't they? (I haven't checked)
I think this is another confusing comment. It's still technically correct but it makes it sound like it's a DenseMap<SDNode, Instruction> when it's now DenseMap<SDNode, GINodeEquiv>. I had to change the values of the map to the GINodeEquiv so I could access the new CheckMMOIsNonAtomic field.


https://reviews.llvm.org/D37445