[PATCH] D64953: GlobalISel: Support physical register inputs in patterns

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: test/TableGen/gisel-physreg-input.td:36-37
+// GISEL-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
+// GISEL-NEXT: // MIs[0] Operand 2
+// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
+// GISEL-NEXT: // (add:{ *:[i32] } GPR32:{ *:[i32] }:$src0, SPECIAL:{ *:[i32] })  =>  (ADD_PHYS:{ *:[i32] } GPR32:{ *:[i32] }:$src0)
----------------
I'm a bit surprised to see that there's no register bank check here. I would have expected specifying a register to be equivalent to specifying a register class containing a single register. I suppose it doesn't have to but it does mean that we can contradict the register bank selector decisions by inserting cross-bank copies it was unaware of.


================
Comment at: test/TableGen/gisel-physreg-input.td:48
+def ADD_PHYS : I<(outs GPR32:$dst), (ins GPR32:$src0),
+    [(set GPR32:$dst, (add GPR32:$src0, SPECIAL))]> {
+  let Uses = [SPECIAL];
----------------
FWIW, I'd prefer registers to be explicitly named. This mostly because of possibility of accidental cases of the FIXME below but it's also because I have a general preference for only having one way to specify a name.


================
Comment at: test/TableGen/gisel-physreg-input.td:52-54
+// Try using the name of the physreg in another operand FIXME: Does
+// this make any sense, or should it be an error? It ends up requiring
+// the two be the same operand.
----------------
I think this is fine so long as the user is choosing the names. I'm not keen on it when a generated name and a chosen name are the same.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:832
 
+  /// A map of anonymouss physical registser operands defined by the matchers
+  /// that may be referenced by the renderers.
----------------
registser -> register


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2319
 
+class CopyPhysRegRenderer : public OperandRenderer {
+protected:
----------------
Could you add a description comment to this class?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2517-2519
+    // TODO: This is encoded as a 64-bit element, but only 16 or 32-bits are
+    // really needed for a physical register reference. We can pack the
+    // register and flags in a single field.
----------------
Yeah, we really ought to compress the table with sleb/uleb where appropriate so that elements have a minimum size of 1 byte. All the common flags would easily fit into 1 byte, and physical register id is at most 3 bytes (when leb encoded) and will often be 1 byte.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3881-3882
 
+    // FIXME: Is this necessary? Seems to avoid problems with interactions with
+    // special WZR handling.
     if (ChildRec->isSubClassOf("Register")) {
----------------
I don't think I understand this comment, it doesn't really give enough context to know why the necessity is in doubt or the problems it avoids


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

https://reviews.llvm.org/D64953