[PATCH] D33590: [globalisel][tablegen] Add support for COPY_TO_REGCLASS.

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Daniel,

Looks globally fine. Mainly two remarks:

- Put more comment on the APIs and classes definition
- Double check why tablegen gives us gpr32 instead of gpr32all

Thanks,
-Quentin



================
Comment at: include/llvm/CodeGen/GlobalISel/Utils.h:36
 
+unsigned constrainRegToClass(MachineRegisterInfo &MRI,
+                             const TargetInstrInfo &TII,
----------------
Add comments on what this is doing.
Don't hesitate to move the comment from the other helper and cross reference to this one.


================
Comment at: lib/CodeGen/GlobalISel/InstructionSelector.cpp:29
+bool InstructionSelector::constrainOperandRegToRegClass(
+    MachineInstr &I, unsigned OpIdx, const TargetRegisterClass *RC,
+    const TargetInstrInfo &TII, const TargetRegisterInfo &TRI,
----------------
TargetRegisterClass &


================
Comment at: lib/CodeGen/GlobalISel/Utils.cpp:33
+                                   MachineInstr &InsertPt, unsigned Reg,
+                                   const TargetRegisterClass *RegClass) {
+  if (!RBI.constrainGenericRegister(Reg, *RegClass, MRI)) {
----------------
Use TargetRegisterClass &


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-bitcast.mir:98
 # CHECK-NEXT:  - { id: 0, class: fpr32 }
-# CHECK-NEXT:  - { id: 1, class: gpr32all }
+# CHECK-NEXT:  - { id: 1, class: gpr32 }
 registers:
----------------
That change is strange. How does tablegen come up with this constraint?
I would have expected that we get the less constrained regclass (gpr32all in that case)


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-bitcast.mir:197
 # CHECK-NEXT:  - { id: 0, class: fpr64 }
-# CHECK-NEXT:  - { id: 1, class: gpr64all }
+# CHECK-NEXT:  - { id: 1, class: gpr64 }
 registers:
----------------
Ditto


================
Comment at: test/TableGen/GlobalISelEmitter.td:476
+// CHECK-NEXT:        ((/* dst */ (MRI.getType(MI0.getOperand(0).getReg()) == (LLT::scalar(32))) &&
+// CHECK-NEXT:         ((&RBI.getRegBankFromRegClass(MyTarget::GPR32RegClass) == RBI.getRegBank(MI0.getOperand(0).getReg(), MRI, TRI))))) &&
+// CHECK-NEXT:        ((/* src1 */ (MRI.getType(MI0.getOperand(1).getReg()) == (LLT::scalar(32))) &&
----------------
Remark: Long term we should be able to use the right regbank directly.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1043
+
+class ConstrainOperandsToDefinitionAction : public MatchAction {
+private:
----------------
Comment on the class' purpose


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1056
+
+class ConstrainOperandToRegClassAction : public MatchAction {
+private:
----------------
Ditto


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1543
   }
-  auto &DstI = Target.getInstruction(DstOp);
+  auto *DstI = &Target.getInstruction(DstOp);
+
----------------
Could you use explicit types here?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1560
+  for (unsigned I = 0; I < DstI->Operands.NumDefs; ++I) {
+    const auto &DstIOperand = DstI->Operands[I];
     DstMIBuilder.addRenderer<CopyRenderer>(InsnMatcher, DstIOperand.Name);
----------------
Ditto


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1568
   for (unsigned I = 0; I != DstINumUses; ++I) {
-    const auto &DstIOperand = DstI.Operands[DstI.Operands.NumDefs + I];
+    const auto &DstIOperand = DstI->Operands[DstI->Operands.NumDefs + I];
 
----------------
Ditto


https://reviews.llvm.org/D33590