[ARM][GISel] PR35965 Constrain RegClasses of nested instructions built from Dst Pattern

Summary:
Apparently, we missed on constraining register classes of VReg-operands of all the instructions
built from a destination pattern but the root (top-level) one. The issue exposed itself
while selecting G_FPTOSI for armv7: the corresponding pattern generates VTOSIZS wrapped
into COPY_TO_REGCLASS, so top-level COPY_TO_REGCLASS gets properly constrained,
while nested VTOSIZS (or rather its destination virtual register to be exact) does not.

Fixing this by issuing GIR_ConstrainSelectedInstOperands for every nested GIR_BuildMI.

https://bugs.llvm.org/show_bug.cgi?id=35965
rdar://problem/36886530

Patch by Roman Tereshin

Reviewers: dsanders, qcolombet, rovka, bogner, aditya_nandakumar, volkan

Reviewed By: dsanders, qcolombet, rovka

Subscribers: aemerson, javed.absar, kristof.beyls, llvm-commits

Differential Revision: https://reviews.llvm.org/D42565

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@323692 91177308-0d34-0410-b5e6-96231b3b80d8

lib/CodeGen/GlobalISel/Utils.cpp

@@ -53,6 +53,24 @@ unsigned llvm::constrainOperandRegClass(
          "PhysReg not implemented");
 
   const TargetRegisterClass *RegClass = TII.getRegClass(II, OpIdx, &TRI, MF);
+  // Some of the target independent instructions, like COPY, may not impose any
+  // register class constraints on some of their operands:
+  if (!RegClass) {
+    assert(!isTargetSpecificOpcode(II.getOpcode()) &&
+           "Only target independent instructions are allowed to have operands "
+           "with no register class constraints");
+    // FIXME: Just bailing out like this here could be not enough, unless we
+    // expect the users of this function to do the right thing for PHIs and
+    // COPY:
+    //   v1 = COPY v0
+    //   v2 = COPY v1
+    // v1 here may end up not being constrained at all. Please notice that to
+    // reproduce the issue we likely need a destination pattern of a selection
+    // rule producing such extra copies, not just an input GMIR with them as
+    // every existing target using selectImpl handles copies before calling it
+    // and they never reach this function.
+    return Reg;
+  }
   return constrainRegToClass(MRI, TII, RBI, InsertPt, Reg, *RegClass);
 }
 
@@ -60,6 +78,8 @@ bool llvm::constrainSelectedInstRegOperands(MachineInstr &I,
                                             const TargetInstrInfo &TII,
                                             const TargetRegisterInfo &TRI,
                                             const RegisterBankInfo &RBI) {
+  assert(!isPreISelGenericOpcode(I.getOpcode()) &&
+         "A selected instruction is expected");
   MachineBasicBlock &MBB = *I.getParent();
   MachineFunction &MF = *MBB.getParent();
   MachineRegisterInfo &MRI = MF.getRegInfo();

lib/Target/ARM/ARMLegalizerInfo.cpp

@@ -134,6 +134,9 @@ ARMLegalizerInfo::ARMLegalizerInfo(const ARMSubtarget &ST) {
   setAction({G_PTRTOINT, s32}, Legal);
   setAction({G_PTRTOINT, 1, p0}, Legal);
 
+  setAction({G_FPTOSI, s32}, Legal);
+  setAction({G_FPTOSI, 1, s32}, Legal);
+
   for (unsigned Op : {G_ASHR, G_LSHR, G_SHL})
     setAction({Op, s32}, Legal);
 

test/CodeGen/ARM/GlobalISel/arm-select-copy_to_regclass-of-fptosi.mir

@@ -0,0 +1,30 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple armv7-gnueabihf -run-pass instruction-select \
+# RUN:     -verify-machineinstrs -o - %s | FileCheck %s
+---
+# Test that we constrain register classes of temporary virtual registers
+# defined by nested instructions built from a Dst Pattern
+#
+# G_FPTOSI selects to a (COPY_TO_REGCLASS (VTOSIZS SPR:$a), GPR), where
+# COPY_TO_REGCLASS doesn't constrain its source register class. It exposes the
+# bug as we create a tmp reg for VTOSIZS' result and don't constrain its
+# register class as COPY_TO_REGCLASS' source (which is fine) nor as VTOSIZS'
+# destination (which is not).
+#
+# https://bugs.llvm.org/show_bug.cgi?id=35965
+name:            test_fptosi
+legalized:       true
+regBankSelected: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: test_fptosi
+    ; CHECK: [[COPY:%[0-9]+]]:spr = COPY %s0
+    ; CHECK: [[VTOSIZS:%[0-9]+]]:spr = VTOSIZS [[COPY]], 14, %noreg
+    ; CHECK: [[COPY1:%[0-9]+]]:gpr = COPY [[VTOSIZS]]
+    ; CHECK: %r0 = COPY [[COPY1]]
+    ; CHECK: MOVPCLR 14, %noreg, implicit %r0
+    %0:fprb(s32) = COPY %s0
+    %1:gprb(s32) = G_FPTOSI %0(s32)
+    %r0 = COPY %1(s32)
+    MOVPCLR 14, %noreg, implicit %r0
+...

test/TableGen/GlobalISelEmitter.td

@@ -308,6 +308,7 @@ def : Pat<(select GPR32:$src1, (complex_rr GPR32:$src2a, GPR32:$src2b),
 // CHECK-NEXT:    GIR_ComplexRenderer, /*InsnID*/1, /*RendererID*/1,
 // CHECK-NEXT:    GIR_ComplexSubOperandRenderer, /*InsnID*/1, /*RendererID*/2, /*SubOperand*/0, // src5a
 // CHECK-NEXT:    GIR_ComplexSubOperandRenderer, /*InsnID*/1, /*RendererID*/2, /*SubOperand*/1, // src5b
+// CHECK-NEXT:    GIR_ConstrainSelectedInstOperands, /*InsnID*/1,
 // CHECK-NEXT:    GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::INSN3,
 // CHECK-NEXT:    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src1

utils/TableGen/GlobalISelEmitter.cpp

@@ -610,8 +610,8 @@ public:
 /// Generates code to check that a match rule matches.
 class RuleMatcher : public Matcher {
 public:
-  using ActionVec = std::vector<std::unique_ptr<MatchAction>>;
-  using action_iterator = ActionVec::iterator;
+  using ActionList = std::list<std::unique_ptr<MatchAction>>;
+  using action_iterator = ActionList::iterator;
 
 protected:
   /// A list of matchers that all need to succeed for the current rule to match.
@@ -622,7 +622,7 @@ protected:
 
   /// A list of actions that need to be taken when all predicates in this rule
   /// have succeeded.
-  ActionVec Actions;
+  ActionList Actions;
 
   using DefinedInsnVariablesMap =
       std::map<const InstructionMatcher *, unsigned>;
@@ -2125,6 +2125,7 @@ public:
   BuildMIAction(unsigned InsnID, const CodeGenInstruction *I)
       : InsnID(InsnID), I(I), Matched(nullptr) {}
 
+  unsigned getInsnID() const { return InsnID; }
   const CodeGenInstruction *getCGI() const { return I; }
 
   void chooseInsnToMutate(RuleMatcher &Rule) {
@@ -3199,7 +3200,7 @@ Expected<BuildMIAction &> GlobalISelEmitter::createAndImportInstructionRenderer(
 
 Expected<action_iterator>
 GlobalISelEmitter::createAndImportSubInstructionRenderer(
-    action_iterator InsertPt, RuleMatcher &M, const TreePatternNode *Dst,
+    const action_iterator InsertPt, RuleMatcher &M, const TreePatternNode *Dst,
     unsigned TempRegID) {
   auto InsertPtOrError = createInstructionRenderer(InsertPt, M, Dst);
 
@@ -3207,7 +3208,6 @@ GlobalISelEmitter::createAndImportSubInstructionRenderer(
 
   if (auto Error = InsertPtOrError.takeError())
     return std::move(Error);
-  InsertPt = InsertPtOrError.get();
 
   BuildMIAction &DstMIBuilder =
       *static_cast<BuildMIAction *>(InsertPtOrError.get()->get());
@@ -3215,10 +3215,13 @@ GlobalISelEmitter::createAndImportSubInstructionRenderer(
   // Assign the result to TempReg.
   DstMIBuilder.addRenderer<TempRegRenderer>(TempRegID, true);
 
-  InsertPtOrError = importExplicitUseRenderers(InsertPt, M, DstMIBuilder, Dst);
+  InsertPtOrError =
+      importExplicitUseRenderers(InsertPtOrError.get(), M, DstMIBuilder, Dst);
   if (auto Error = InsertPtOrError.takeError())
     return std::move(Error);
 
+  M.insertAction<ConstrainOperandsToDefinitionAction>(InsertPt,
+                                                      DstMIBuilder.getInsnID());
   return InsertPtOrError.get();
 }