From 8c0577af1a68b51e57b36bbe5b63837f402ad600 Mon Sep 17 00:00:00 2001 From: Michael Maitland Date: Tue, 31 Oct 2023 18:33:47 -0700 Subject: [PATCH 1/6] [CodeGen][MachineVerifier] Use TypeSize instead of unsigned for getRegSizeInBits This patch changes getRegSizeInBits to return a TypeSize instead of an unsigned in the case that a virtual register has a scalable LLT. In the case that register is physical, a Fixed TypeSize is returned. The MachineVerifier pass is updated to allow copies between fixed and scalable operands as long as the Src size will fit into the Dest size. This is a precommit which will be stacked on by a change to GISel to generate COPYs with a scalable destination but a fixed size source. --- .../include/llvm/CodeGen/TargetRegisterInfo.h | 6 ++--- llvm/lib/CodeGen/MachineVerifier.cpp | 22 ++++++++++++++----- llvm/lib/CodeGen/TargetRegisterInfo.cpp | 19 ++++++++-------- 3 files changed, 29 insertions(+), 18 deletions(-) diff --git a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h index 337fab735a095..4fb6ba7c26930 100644 --- a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h +++ b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h @@ -283,8 +283,8 @@ class TargetRegisterInfo : public MCRegisterInfo { // DenseMapInfo uses -1u and -2u. /// Return the size in bits of a register from class RC. - unsigned getRegSizeInBits(const TargetRegisterClass &RC) const { - return getRegClassInfo(RC).RegSize; + TypeSize getRegSizeInBits(const TargetRegisterClass &RC) const { + return TypeSize::Fixed(getRegClassInfo(RC).RegSize); } /// Return the size in bytes of the stack slot allocated to hold a spilled @@ -858,7 +858,7 @@ class TargetRegisterInfo : public MCRegisterInfo { const TargetRegisterClass *RC) const = 0; /// Returns size in bits of a phys/virtual/generic register. - unsigned getRegSizeInBits(Register Reg, const MachineRegisterInfo &MRI) const; + TypeSize getRegSizeInBits(Register Reg, const MachineRegisterInfo &MRI) const; /// Get the weight in units of pressure for this register unit. virtual unsigned getRegUnitWeight(unsigned RegUnit) const = 0; diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp index dadaf60fa09da..9837a93d83399 100644 --- a/llvm/lib/CodeGen/MachineVerifier.cpp +++ b/llvm/lib/CodeGen/MachineVerifier.cpp @@ -1937,8 +1937,9 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) { // If we have only one valid type, this is likely a copy between a virtual // and physical register. - unsigned SrcSize = 0; - unsigned DstSize = 0; + TypeSize SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI); + TypeSize DstSize = TRI->getRegSizeInBits(DstReg, *MRI); + if (SrcReg.isPhysical() && DstTy.isValid()) { const TargetRegisterClass *SrcRC = TRI->getMinimalPhysRegClassLLT(SrcReg, DstTy); @@ -1946,7 +1947,7 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) { SrcSize = TRI->getRegSizeInBits(*SrcRC); } - if (SrcSize == 0) + if (SrcSize.isZero()) SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI); if (DstReg.isPhysical() && SrcTy.isValid()) { @@ -1956,10 +1957,21 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) { DstSize = TRI->getRegSizeInBits(*DstRC); } - if (DstSize == 0) + if (DstSize.isZero()) DstSize = TRI->getRegSizeInBits(DstReg, *MRI); - if (SrcSize != 0 && DstSize != 0 && SrcSize != DstSize) { + // If the Dst is scalable and the Src is fixed, then the Dst can only hold + // the Src if the minimum size Dst can hold is at least as big as Src. + if (DstSize.isScalable() && !SrcSize.isScalable() && + DstSize.getKnownMinValue() <= SrcSize.getFixedValue()) + break; + // If the Src is scalable and the Dst is fixed, then Dest can only hold + // the Src is known to fit in Dest + if (SrcSize.isScalable() && !DstSize.isScalable() && + TypeSize::isKnownLE(DstSize, SrcSize)) + break; + + if (SrcSize.isNonZero() && DstSize.isNonZero() && SrcSize != DstSize) { if (!DstOp.getSubReg() && !SrcOp.getSubReg()) { report("Copy Instruction is illegal with mismatching sizes", MI); errs() << "Def Size = " << DstSize << ", Src Size = " << SrcSize diff --git a/llvm/lib/CodeGen/TargetRegisterInfo.cpp b/llvm/lib/CodeGen/TargetRegisterInfo.cpp index 1bb35f40facfd..c50b1cf942271 100644 --- a/llvm/lib/CodeGen/TargetRegisterInfo.cpp +++ b/llvm/lib/CodeGen/TargetRegisterInfo.cpp @@ -499,7 +499,7 @@ bool TargetRegisterInfo::regmaskSubsetEqual(const uint32_t *mask0, return true; } -unsigned +TypeSize TargetRegisterInfo::getRegSizeInBits(Register Reg, const MachineRegisterInfo &MRI) const { const TargetRegisterClass *RC{}; @@ -508,16 +508,15 @@ TargetRegisterInfo::getRegSizeInBits(Register Reg, // Instead, we need to access a register class that contains Reg and // get the size of that register class. RC = getMinimalPhysRegClass(Reg); - } else { - LLT Ty = MRI.getType(Reg); - unsigned RegSize = Ty.isValid() ? Ty.getSizeInBits() : 0; - // If Reg is not a generic register, query the register class to - // get its size. - if (RegSize) - return RegSize; - // Since Reg is not a generic register, it must have a register class. - RC = MRI.getRegClass(Reg); + assert(RC && "Unable to deduce the register class"); + return getRegSizeInBits(*RC); } + LLT Ty = MRI.getType(Reg); + if (Ty.isValid()) + return Ty.getSizeInBits(); + + // Since Reg is not a generic register, it may have a register class. + RC = MRI.getRegClass(Reg); assert(RC && "Unable to deduce the register class"); return getRegSizeInBits(*RC); } From 7d1aaced6950cce52e35ff7550393ff9708bbaac Mon Sep 17 00:00:00 2001 From: Michael Maitland Date: Tue, 31 Oct 2023 18:57:14 -0700 Subject: [PATCH 2/6] Avoid duplicate computation --- llvm/lib/CodeGen/MachineVerifier.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp index 9837a93d83399..ca0c963a7aa71 100644 --- a/llvm/lib/CodeGen/MachineVerifier.cpp +++ b/llvm/lib/CodeGen/MachineVerifier.cpp @@ -1939,7 +1939,6 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) { // and physical register. TypeSize SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI); TypeSize DstSize = TRI->getRegSizeInBits(DstReg, *MRI); - if (SrcReg.isPhysical() && DstTy.isValid()) { const TargetRegisterClass *SrcRC = TRI->getMinimalPhysRegClassLLT(SrcReg, DstTy); @@ -1947,9 +1946,6 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) { SrcSize = TRI->getRegSizeInBits(*SrcRC); } - if (SrcSize.isZero()) - SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI); - if (DstReg.isPhysical() && SrcTy.isValid()) { const TargetRegisterClass *DstRC = TRI->getMinimalPhysRegClassLLT(DstReg, SrcTy); @@ -1957,9 +1953,6 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) { DstSize = TRI->getRegSizeInBits(*DstRC); } - if (DstSize.isZero()) - DstSize = TRI->getRegSizeInBits(DstReg, *MRI); - // If the Dst is scalable and the Src is fixed, then the Dst can only hold // the Src if the minimum size Dst can hold is at least as big as Src. if (DstSize.isScalable() && !SrcSize.isScalable() && From 166017cfb1958a842f80a5363452917b985cbd61 Mon Sep 17 00:00:00 2001 From: Michael Maitland Date: Tue, 31 Oct 2023 20:39:46 -0700 Subject: [PATCH 3/6] Add MachineVerifier test case --- llvm/test/MachineVerifier/copy-scalable.mir | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 llvm/test/MachineVerifier/copy-scalable.mir diff --git a/llvm/test/MachineVerifier/copy-scalable.mir b/llvm/test/MachineVerifier/copy-scalable.mir new file mode 100644 index 0000000000000..f4088f7aed34d --- /dev/null +++ b/llvm/test/MachineVerifier/copy-scalable.mir @@ -0,0 +1,23 @@ +# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 3 +# RUN: llc -mtriple=riscv64 -o - -global-isel -run-pass=none -verify-machineinstrs %s | FileCheck %s +# REQUIRES: riscv64-registered-target + +--- +name: test_copy_fixed_to_scalable +legalized: true +regBankSelected: false +selected: false +tracksRegLiveness: true +registers: + - { id: 0, class: _, preferred-register: '' } +liveins: +body: | + bb.0: + liveins: $v8 + + ; CHECK-LABEL: name: test_copy_fixed_to_scalable + ; CHECK: liveins: $v8 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: [[COPY:%[0-9]+]]:_() = COPY $v8 + %0:_() = COPY $v8 +... From 0f0e8ab6e24e5f099281be3100ab3f111c234c9b Mon Sep 17 00:00:00 2001 From: Michael Maitland Date: Tue, 31 Oct 2023 20:50:07 -0700 Subject: [PATCH 4/6] fix failing test --- llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll index 5dd62de8a6bc4..a3a913d8ce02d 100644 --- a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll +++ b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll @@ -22,7 +22,7 @@ entry: ret %a } -; FALLBACK-WITH-REPORT-ERR: remark: :0:0: unable to translate instruction{{.*}}scalable_inst +; FALLBACK-WITH-REPORT-ERR: remark: :0:0: unable to translate instruction: call: ; FALLBACK-WITH-REPORT-OUT-LABEL: scalable_inst define @scalable_inst(i64 %0) nounwind { entry: @@ -35,7 +35,7 @@ entry: ret %a } -; FALLBACK-WITH-REPORT-ERR: remark: :0:0: unable to translate instruction{{.*}}scalable_alloca +; FALLBACK-WITH-REPORT-ERR: remark: :0:0: unable to translate instruction: alloca: ; FALLBACK-WITH-REPORT-OUT-LABEL: scalable_alloca define void @scalable_alloca() #1 { %local0 = alloca From 10d2840aa7091ba9744601b7e2c733a533377a28 Mon Sep 17 00:00:00 2001 From: Michael Maitland Date: Thu, 2 Nov 2023 19:44:03 -0700 Subject: [PATCH 5/6] remove impossible case --- llvm/lib/CodeGen/MachineVerifier.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp index ca0c963a7aa71..4ebc13c9c34f9 100644 --- a/llvm/lib/CodeGen/MachineVerifier.cpp +++ b/llvm/lib/CodeGen/MachineVerifier.cpp @@ -1958,11 +1958,6 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) { if (DstSize.isScalable() && !SrcSize.isScalable() && DstSize.getKnownMinValue() <= SrcSize.getFixedValue()) break; - // If the Src is scalable and the Dst is fixed, then Dest can only hold - // the Src is known to fit in Dest - if (SrcSize.isScalable() && !DstSize.isScalable() && - TypeSize::isKnownLE(DstSize, SrcSize)) - break; if (SrcSize.isNonZero() && DstSize.isNonZero() && SrcSize != DstSize) { if (!DstOp.getSubReg() && !SrcOp.getSubReg()) { From 624c94517e8c77b71df8cb9ba97727261a100f89 Mon Sep 17 00:00:00 2001 From: Michael Maitland Date: Fri, 3 Nov 2023 07:37:39 -0700 Subject: [PATCH 6/6] Only allow copy from physical -> virtual --- llvm/lib/CodeGen/MachineVerifier.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp index 4ebc13c9c34f9..dc15f0d3b8423 100644 --- a/llvm/lib/CodeGen/MachineVerifier.cpp +++ b/llvm/lib/CodeGen/MachineVerifier.cpp @@ -1953,9 +1953,11 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) { DstSize = TRI->getRegSizeInBits(*DstRC); } - // If the Dst is scalable and the Src is fixed, then the Dst can only hold - // the Src if the minimum size Dst can hold is at least as big as Src. - if (DstSize.isScalable() && !SrcSize.isScalable() && + // If this is a copy from physical register to virtual register, and if the + // Dst is scalable and the Src is fixed, then the Dst can only hold the Src + // if the minimum size Dst can hold is at least as big as Src. + if (SrcReg.isPhysical() && DstReg.isVirtual() && DstSize.isScalable() && + !SrcSize.isScalable() && DstSize.getKnownMinValue() <= SrcSize.getFixedValue()) break;