From ca71b34717817be55d4bae55207b959c5bc7bcc8 Mon Sep 17 00:00:00 2001 From: Daniel Sanders Date: Mon, 29 Jan 2018 21:09:12 +0000 Subject: [PATCH] [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 | 20 +++++++++++++++ lib/Target/ARM/ARMLegalizerInfo.cpp | 3 +++ .../arm-select-copy_to_regclass-of-fptosi.mir | 30 ++++++++++++++++++++++ test/TableGen/GlobalISelEmitter.td | 1 + utils/TableGen/GlobalISelEmitter.cpp | 15 ++++++----- 5 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 test/CodeGen/ARM/GlobalISel/arm-select-copy_to_regclass-of-fptosi.mir diff --git a/lib/CodeGen/GlobalISel/Utils.cpp b/lib/CodeGen/GlobalISel/Utils.cpp index 9f8440f3316..35ad433a97d 100644 --- a/lib/CodeGen/GlobalISel/Utils.cpp +++ b/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(); diff --git a/lib/Target/ARM/ARMLegalizerInfo.cpp b/lib/Target/ARM/ARMLegalizerInfo.cpp index 604dea14e9c..690017df685 100644 --- a/lib/Target/ARM/ARMLegalizerInfo.cpp +++ b/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); diff --git a/test/CodeGen/ARM/GlobalISel/arm-select-copy_to_regclass-of-fptosi.mir b/test/CodeGen/ARM/GlobalISel/arm-select-copy_to_regclass-of-fptosi.mir new file mode 100644 index 00000000000..9045f31ff75 --- /dev/null +++ b/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 +... diff --git a/test/TableGen/GlobalISelEmitter.td b/test/TableGen/GlobalISelEmitter.td index fe2f355f871..a6d61d61f6a 100644 --- a/test/TableGen/GlobalISelEmitter.td +++ b/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 diff --git a/utils/TableGen/GlobalISelEmitter.cpp b/utils/TableGen/GlobalISelEmitter.cpp index 865a01a868e..8f5a07c5a82 100644 --- a/utils/TableGen/GlobalISelEmitter.cpp +++ b/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>; - using action_iterator = ActionVec::iterator; + using ActionList = std::list>; + 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; @@ -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 GlobalISelEmitter::createAndImportInstructionRenderer( Expected 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(InsertPtOrError.get()->get()); @@ -3215,10 +3215,13 @@ GlobalISelEmitter::createAndImportSubInstructionRenderer( // Assign the result to TempReg. DstMIBuilder.addRenderer(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(InsertPt, + DstMIBuilder.getInsnID()); return InsertPtOrError.get(); } -- 2.11.0