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

[rtereshin]

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

[rovka]

LGTM, thanks!

[dsanders]

LGTM with a nit. Thanks for looking into this.

lib/CodeGen/GlobalISel/Utils.cpp

56–59 ↗

(On Diff #131548)

This is a bit more general than I'd like. I agree that TargetOpcodes like COPY shouldn't fail, but I think GenericOpcodes and target specific instructions should. Could you assert(!isPreISelGenericOpcode() && !isTargetSpecificOpcode()) inside this if-statement?

[rtereshin]

lib/CodeGen/GlobalISel/Utils.cpp

56–59 ↗

(On Diff #131548)

Sure! That's just what I needed to know: how to describe in a generic way all the instructions like COPY here, huge thanks!

[rtereshin]

@dsanders Hi Daniel, could you please commit this for me?

[qcolombet]

lib/CodeGen/GlobalISel/Utils.cpp

62 ↗

(On Diff #131630)

I believe bailing out won't be enough generally speaking, unless we expect the users of this method to do the right thing for PHIs and COPY.

E.g., by bailing out on COPYs we could end up with unconstrained VRegs in cases like this:
v1 = COPY v1 won't get constrained
v2 = COPY v1 <-- v2 will be constrained by its users

Usually that means we have useless COPYs though.

56–59 ↗

(On Diff #131548)

I believe this part should go in separately.
This is not related to the TableGen change.

test/CodeGen/ARM/GlobalISel/arm-select-fptosi-pr35965.mir

2 ↗

(On Diff #131630)

Could you use something more descriptive than prxxx in the file name or just drop it?

Reference the PR in the comment here though.

[rtereshin]

lib/CodeGen/GlobalISel/Utils.cpp

62 ↗

(On Diff #131630)

True, but this patch doesn't make this problem either better or worse. Maybe solve it in a different patch, but commit this one so we don't block ARM folks?

56–59 ↗

(On Diff #131548)

It is related in a sense that the TableGen change would break GlobalISel for some of the targets w/o this change.

test/CodeGen/ARM/GlobalISel/arm-select-fptosi-pr35965.mir

2 ↗

(On Diff #131630)

Sure! I looked around, saw names like this, and thought it's a standard practice to follow =) Glad to know that it's not.

[rtereshin]

lib/CodeGen/GlobalISel/Utils.cpp

56–59 ↗

(On Diff #131548)

Looks like if we put it in a separate patch it will go w/o a test case, as to catch the problem we need to call constrainSelectedInstRegOperands on a copy, and that never currently happens as:

1. selectImpl currently doesn't support COPY at all, and every target handles them before calling selectImpl
2. If COPY is introduced by a destination pattern nothing constrains it as we weren't constraining nested instructions before the tablegen part of this patch.
3. (2) never caused a problem as existing targets (apparently) don't have patterns like this.

So I don't quite see an a reasonably simple way to come up with a test for this that actually fails before the fix w/o introducing a fake testing target.

In other words, this part of this patch can't have a test case w/o the tablegen part, but the tablegen part can't go in w/o this as it will cause existing tests to fail.

[qcolombet]

Got a chat with Roman off-line and turns out the problem I mentioned is worked around by everything single target, so indeed no test cases!
LGTM then

llvm/trunk/lib/CodeGen/GlobalISel/Utils.cpp

unsigned llvm::constrainOperandRegClass(
     MachineRegisterInfo &MRI, const TargetInstrInfo &TII,
     const RegisterBankInfo &RBI, MachineInstr &InsertPt, const MCInstrDesc &II,
     unsigned Reg, unsigned OpIdx) {
   // Assume physical registers are properly constrained.
   assert(TargetRegisterInfo::isVirtualRegister(Reg) &&
          "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);
 }
 
 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();
 
   for (unsigned OpI = 0, OpE = I.getNumExplicitOperands(); OpI != OpE; ++OpI) {
     MachineOperand &MO = I.getOperand(OpI);
 
     // There's nothing to be done on non-register operands.

llvm/trunk/lib/Target/ARM/ARMLegalizerInfo.cpp

ARMLegalizerInfo::ARMLegalizerInfo(const ARMSubtarget &ST) {
   }
 
   setAction({G_INTTOPTR, p0}, Legal);
   setAction({G_INTTOPTR, 1, s32}, Legal);
 
   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);
 
   setAction({G_GEP, p0}, Legal);
   setAction({G_GEP, 1, s32}, Legal);
 
   setAction({G_SELECT, s32}, Legal);
   setAction({G_SELECT, p0}, Legal);

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

+# 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
+...

llvm/trunk/test/TableGen/GlobalISelEmitter.td

 // CHECK-NEXT:    // (select:{ *:[i32] } GPR32:{ *:[i32] }:$src1, (complex_rr:{ *:[i32] } GPR32:{ *:[i32] }:$src2a, GPR32:{ *:[i32] }:$src2b), (select:{ *:[i32] } GPR32:{ *:[i32] }:$src3, complex:{ *:[i32] }:$src4, (complex:{ *:[i32] } i32imm:{ *:[i32] }:$src5a, i32imm:{ *:[i32] }:$src5b)))  =>  (INSN3:{ *:[i32] } GPR32:{ *:[i32] }:$src1, GPR32:{ *:[i32] }:$src2b, GPR32:{ *:[i32] }:$src2a, (INSN4:{ *:[i32] } GPR32:{ *:[i32] }:$src3, complex:{ *:[i32] }:$src4, i32imm:{ *:[i32] }:$src5a, i32imm:{ *:[i32] }:$src5b))
 // CHECK-NEXT:    GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s32,
 // CHECK-NEXT:    GIR_BuildMI, /*InsnID*/1, /*Opcode*/MyTarget::INSN4,
 // CHECK-NEXT:    GIR_AddTempRegister, /*InsnID*/1, /*TempRegID*/0, /*TempRegFlags*/RegState::Define,
 // CHECK-NEXT:    GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/1, /*OpIdx*/1, // src3
 // 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
 // CHECK-NEXT:    GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // src2b
 // CHECK-NEXT:    GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src2a
 // CHECK-NEXT:    GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/0,
 // CHECK-NEXT:    GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:    GIR_ConstrainSelectedInstOperands, /*InsnID*/0,

llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp

std::unique_ptr<PredicateMatcher> forgetFirstCondition() override {
     assert(0 && "Groups should be formed maximal for now");
     llvm_unreachable("No need for this for now");
   }
 };
 
 /// 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.
   /// FIXME: This currently supports a single match position but could be
   /// extended to support multiple positions to support div/rem fusion or
   /// load-multiple instructions.
   std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
 
   /// 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>;
 
   /// A map of instruction matchers to the local variables created by
   /// emitCaptureOpcodes().
   DefinedInsnVariablesMap InsnVariableIDs;
 
bool canMutate(RuleMatcher &Rule, const InstructionMatcher *Insn) const {
 
     return true;
   }
 
 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) {
     for (const auto *MutateCandidate : Rule.mutatable_insns()) {
       if (canMutate(Rule, MutateCandidate)) {
         // Take the first one we're offered that we're able to mutate.
         Rule.reserveInsnMatcherForMutation(MutateCandidate);
         Matched = MutateCandidate;
if (auto Error = importExplicitUseRenderers(InsertPt, M, DstMIBuilder, Dst)
                        .takeError())
     return std::move(Error);
 
   return DstMIBuilder;
 }
 
 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);
 
   // TODO: Assert there's exactly one result.
 
   if (auto Error = InsertPtOrError.takeError())
     return std::move(Error);
-  InsertPt = InsertPtOrError.get();
 
   BuildMIAction &DstMIBuilder =
       *static_cast<BuildMIAction *>(InsertPtOrError.get()->get());
 
   // 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();
 }
 
 Expected<action_iterator> GlobalISelEmitter::createInstructionRenderer(
     action_iterator InsertPt, RuleMatcher &M, const TreePatternNode *Dst) {
   Record *DstOp = Dst->getOperator();
   if (!DstOp->isSubClassOf("Instruction")) {
     if (DstOp->isSubClassOf("ValueType"))