Skip to content

[CodeGen][MachineVerifier] Use TypeSize instead of unsigned for getRe… #70881

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 7, 2023
Merged

[CodeGen][MachineVerifier] Use TypeSize instead of unsigned for getRe… #70881

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8c0577a
[CodeGen][MachineVerifier] Use TypeSize instead of unsigned for getRe…
michaelmaitland Nov 1, 2023
7d1aace
Avoid duplicate computation
michaelmaitland Nov 1, 2023
166017c
Add MachineVerifier test case
michaelmaitland Nov 1, 2023
0f0e8ab
fix failing test
michaelmaitland Nov 1, 2023
10d2840
remove impossible case
michaelmaitland Nov 3, 2023
624c945
Only allow copy from physical -> virtual
michaelmaitland Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions llvm/include/llvm/CodeGen/TargetRegisterInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,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
Expand Down Expand Up @@ -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;
Expand Down
21 changes: 13 additions & 8 deletions llvm/lib/CodeGen/MachineVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1937,29 +1937,34 @@ 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);
if (SrcRC)
SrcSize = TRI->getRegSizeInBits(*SrcRC);
}

if (SrcSize == 0)
SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI);

if (DstReg.isPhysical() && SrcTy.isValid()) {
const TargetRegisterClass *DstRC =
TRI->getMinimalPhysRegClassLLT(DstReg, SrcTy);
if (DstRC)
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() &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that sounds OK to me, with the way scalable vector currently work. We can adjust it later if it becomes an issue.

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
Expand Down
19 changes: 9 additions & 10 deletions llvm/lib/CodeGen/TargetRegisterInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{};
Expand All @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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>
Expand Down
23 changes: 23 additions & 0 deletions llvm/test/MachineVerifier/copy-scalable.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
...