[CodeGen][MachineVerifier] Use TypeSize instead of unsigned for getRe… #70881
Conversation
michaelmaitland
requested review from
arsenm,
MaskRay,
mshockwave and
topperc
|
@llvm/pr-subscribers-llvm-globalisel Author: Michael Maitland (michaelmaitland) Changes…gSizeInBits 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. Full diff: https://github.com/llvm/llvm-project/pull/70881.diff 6 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
index 5bf27e40eee8909..3f64bf972daf21e 100644
--- a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
@@ -278,8 +278,8 @@ class TargetRegisterInfo : public MCRegisterInfo {
// DenseMapInfo<unsigned> 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
@@ -853,7 +853,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/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
index c01b34d6f490b0e..0da664e935f5442 100644
--- a/llvm/lib/CodeGen/MIRParser/MIParser.cpp
+++ b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
@@ -1946,12 +1946,28 @@ bool MIParser::parseLowLevelType(StringRef::iterator Loc, LLT &Ty) {
// Now we're looking for a vector.
if (Token.isNot(MIToken::less))
- return error(Loc,
- "expected sN, pA, <M x sN>, or <M x pA> for GlobalISel type");
+ return error(Loc, "expected sN, pA, <M x sN>, <M x pA>, <vscale x M x sN>, "
+ "or <vscale x M x pA> for GlobalISel type");
lex();
+ bool HasVScale = Token.stringValue() == "vscale";
+ if (HasVScale) {
+ lex();
+ if (Token.stringValue() != "x")
+ return error("expected <vscale x M x sN> or <vscale x M x pA>");
+ lex();
+ }
+
+ auto GetError = [&](bool HasVScale, StringRef::iterator Loc) {
+ if (HasVScale)
+ return error(
+ Loc, "expected <vscale x M x sN> or <vscale M x pA> for vector type");
+ else
+ return error(Loc, "expected <M x sN> or <M x pA> for vector type");
+ };
+
if (Token.isNot(MIToken::IntegerLiteral))
- return error(Loc, "expected <M x sN> or <M x pA> for vector type");
+ return GetError(HasVScale, Loc);
uint64_t NumElements = Token.integerValue().getZExtValue();
if (!verifyVectorElementCount(NumElements))
return error("invalid number of vector elements");
@@ -1959,11 +1975,12 @@ bool MIParser::parseLowLevelType(StringRef::iterator Loc, LLT &Ty) {
lex();
if (Token.isNot(MIToken::Identifier) || Token.stringValue() != "x")
- return error(Loc, "expected <M x sN> or <M x pA> for vector type");
+ return GetError(HasVScale, Loc);
lex();
if (Token.range().front() != 's' && Token.range().front() != 'p')
- return error(Loc, "expected <M x sN> or <M x pA> for vector type");
+ return GetError(HasVScale, Loc);
+
StringRef SizeStr = Token.range().drop_front();
if (SizeStr.size() == 0 || !llvm::all_of(SizeStr, isdigit))
return error("expected integers after 's'/'p' type character");
@@ -1981,14 +1998,18 @@ bool MIParser::parseLowLevelType(StringRef::iterator Loc, LLT &Ty) {
Ty = LLT::pointer(AS, DL.getPointerSizeInBits(AS));
} else
- return error(Loc, "expected <M x sN> or <M x pA> for vector type");
+ return GetError(HasVScale, Loc);
lex();
if (Token.isNot(MIToken::greater))
- return error(Loc, "expected <M x sN> or <M x pA> for vector type");
+ return GetError(HasVScale, Loc);
+
lex();
- Ty = LLT::fixed_vector(NumElements, Ty);
+ if (HasVScale)
+ Ty = LLT::scalable_vector(NumElements, Ty);
+ else
+ Ty = LLT::fixed_vector(NumElements, Ty);
return false;
}
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index dadaf60fa09da04..ca0c963a7aa7158 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -1937,8 +1937,8 @@ 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,9 +1946,6 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
SrcSize = TRI->getRegSizeInBits(*SrcRC);
}
- if (SrcSize == 0)
- SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI);
-
if (DstReg.isPhysical() && SrcTy.isValid()) {
const TargetRegisterClass *DstRC =
TRI->getMinimalPhysRegClassLLT(DstReg, SrcTy);
@@ -1956,10 +1953,18 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
DstSize = TRI->getRegSizeInBits(*DstRC);
}
- if (DstSize == 0)
- 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() &&
+ 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 != 0 && DstSize != 0 && SrcSize != DstSize) {
+ 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 1bb35f40facfd0f..c50b1cf9422717a 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);
}
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll
index 5dd62de8a6bc415..a3a913d8ce02d83 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 <vscale x 1 x i8> %a
}
-; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction{{.*}}scalable_inst
+; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction: call:
; FALLBACK-WITH-REPORT-OUT-LABEL: scalable_inst
define <vscale x 1 x i8> @scalable_inst(i64 %0) nounwind {
entry:
@@ -35,7 +35,7 @@ entry:
ret <vscale x 1 x i8> %a
}
-; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction{{.*}}scalable_alloca
+; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction: alloca:
; FALLBACK-WITH-REPORT-OUT-LABEL: scalable_alloca
define void @scalable_alloca() #1 {
%local0 = alloca <vscale x 16 x i8>
diff --git a/llvm/test/MachineVerifier/copy-scalable.mir b/llvm/test/MachineVerifier/copy-scalable.mir
new file mode 100644
index 000000000000000..f4088f7aed34dde
--- /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]+]]:_(<vscale x 1 x s8>) = COPY $v8
+ %0:_(<vscale x 1 x s8>) = COPY $v8
+...
|
michaelmaitland
force-pushed
the
fixed-to-scalable-copy
branch
from
e6eb7ed
to
8f06894
Compare
|
Do you plan to mark register sizes as scalable? I was looking at the bare minimum needed to get scalable vectors working for GlobalISel last week, and got something that worked, but would likely hit a lot of other problems. I wasn't sure if it would end up being necessary to mark the registers as scalable, the patch I had was still very much work-in-progress. |
I hadn't really thought much into that idea. I posted a patch for call lowering here: #70882. I hadn't come across the need to mark the register sizes as scalable. I think what I had in mind is that scalable vectors map to some fixed size LMUL and SEW grouping on RISCV because of that, we'd lower into those types and not have to think much harder about scalable vectors. What did you have in mind? Do you see a need to mark the reg sizes as scalable?
If you have a patch/patches I am more than happy to review. If you want to have a chat about what scalable vectors in GISel will/should look like, we can set up a call. |
…gSizeInBits 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.
michaelmaitland
force-pushed
the
fixed-to-scalable-copy
branch
from
8f06894
to
0f0e8ab
Compare
llvm/lib/CodeGen/MachineVerifier.cpp
Outdated
| 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() && |
There was a problem hiding this comment.
@arsenm I think I should remove this case. I'm not testing for it and I'm pretty sure its impossible for isKnownLE to ever be true here. WDYT?
There was a problem hiding this comment.
Copy is only supposed to allow equal sized operands, but that starts getting fuzzy with physical registers. Is it really supposed to allow these mixed cases for scalable vectors?
There was a problem hiding this comment.
I agree that copys should be of the same size. As far as I understand the scalable vector registers have never been marked as "Scalable" though, only being given a size equal to the KnownMinValue.
The patch at davemgreen@07e9bdd was enough to make the test case there work for a simple add. It went the route of marking the vector registers as scalable. I don't think that is necessary to make things work though, in that I can remove the Scalable defs from RegisterClasses and so long as it gets past the verifier it works for that small example still. SDAG never needed the registers to be scalable.
The sizes of some of the SVE scalable register types is a bit odd in places though, I was expecting them to all be vscale*128bit, and Im not sure how RISCV scalable vectors are defined.
There was a problem hiding this comment.
We do need some ability to copy physical registers into virtual registers and vice versa.
The way we accomplish this, I am not sure, and I am interested in having a discussion on it. I have updated the code here to allow copy from physical -> virtual when we know the min size is at least as big. However, I am not confident that this is the correct approach. The first reason I am not sure this is the correct approach is because this dual for return values will not work:
if (SrcReg.isVirtual() && DstReg.isPhysical() &&
SrcSize.isScalable() && !DstSize.isScalable() &&
TypeSize::isKnownLE(DstSize, SrcSize)
The problem here is that TypeSize::isKnownLE(DstSize, SrcSize) will always be false.
The second reason I am not sure this is the correct approach is because it isn't clear to me what happens to elements in the scalable vector that are past the size of the physical register. For example, v8 in the test case below reports a size of 64 bits, but %0 reports a size of vscale x 8. If vscale is bigger than 8, then I'm not sure what goes into the rest of the elements.
Does anyone have any opinion on a better approach?
@davemgreen, does marking the physical registers as scalable solve this problem? IIUC marking v8 here as scalable would mean that it has size vscale x 64? I'm not sure that this would match with the vscale x 8 size that we're using for the virtual register though.
There was a problem hiding this comment.
@davemgreen do you plan to PR davemgreen@07e9bdd? I am pretty sure at least some of the unsigned -> TypeSize changes will be useful when it comes to regbank select.
There was a problem hiding this comment.
Hello - I don't have any immediate plans to push that forward, it would take some cleanup to get it into a good state. Feel free to take/reuse any part that is useful to you.
As for the verifier check, as far as I understand that we have it at the moment, the "Size" of the physical register class should be the fixed size without being marked as scalable, and all the types added to it should be the same size or smaller ignoring the scalability.
There was a problem hiding this comment.
As for the verifier check, as far as I understand that we have it at the moment, the "Size" of the physical register class should be the fixed size without being marked as scalable, and all the types added to it should be the same size or smaller ignoring the scalability.
I believe what is implemented here essentially does this, without marking the physical register as scalable. Here, we check that the MinKnownValue is less than or equal to the fixed value. This is the same as checking that the "size of the physical register should be the fixed size without being marked as scalable".
I think this patch here is less invasive than marking physical registers as scalable and I propose that we take this patch and can discuss making physical registers scalable in the future. WDYT?
There was a problem hiding this comment.
Yeah that sounds OK to me, with the way scalable vector currently work. We can adjust it later if it becomes an issue.
Scalable vector types from LLVM IR are lowered into physical vector registers in MIR based on calling convention. This patch is stacked on llvm#70881.
…gSizeInBits
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.
This patch is stacked on #70893 for the ability to use scalable vector types in MIR tests.