[ARM] Fix problems with register list in vscclrm #111825
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The register list in vscclrm is unusual in two ways: * The encoded size can be zero, meaning the list contains only vpr. * Double-precision registers past d15 are permitted even when the subtarget doesn't have them, they are instead ignored when the instruction executes. Fixing this also incidentally changes a vlldm/vlstm error message: when the first register is in the range d16-d31 we now get the "operand must be exactly..." error instead of "register expected".
john-brawn-arm
requested review from
AlfieRichardsArm,
sivan-shani,
statham-arm and
topperc
Member
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-arm Author: John Brawn (john-brawn-arm) ChangesThe register list in vscclrm is unusual in two ways:
Fixing this also incidentally changes a vlldm/vlstm error message: when the first register is in the range d16-d31 we now get the "operand must be exactly..." error instead of "register expected". Full diff: https://github.com/llvm/llvm-project/pull/111825.diff 6 Files Affected:
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index 75fb90477f8854..1cf9844be2695c 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -3810,6 +3810,10 @@ class ARMOperand : public MCParsedAsmOperand {
Kind = k_FPSRegisterListWithVPR;
else
Kind = k_SPRRegisterList;
+ } else if (Regs.front().second == ARM::VPR) {
+ assert(Regs.size() == 1 &&
+ "Register list starting with VPR expected to only contain VPR");
+ Kind = k_FPSRegisterListWithVPR;
}
if (Kind == k_RegisterList && Regs.back().second == ARM::APSR)
@@ -4617,15 +4621,15 @@ bool ARMAsmParser::parseRegisterList(OperandVector &Operands, bool EnforceOrder,
// Check the first register in the list to see what register class
// this is a list of.
- MCRegister Reg = tryParseRegister();
+ MCRegister Reg = tryParseRegister(AllowOutOfBoundReg);
if (!Reg)
return Error(RegLoc, "register expected");
if (!AllowRAAC && Reg == ARM::RA_AUTH_CODE)
return Error(RegLoc, "pseudo-register not allowed");
- // The reglist instructions have at most 16 registers, so reserve
+ // The reglist instructions have at most 32 registers, so reserve
// space for that many.
int EReg = 0;
- SmallVector<std::pair<unsigned, MCRegister>, 16> Registers;
+ SmallVector<std::pair<unsigned, MCRegister>, 32> Registers;
// Allow Q regs and just interpret them as the two D sub-registers.
if (ARMMCRegisterClasses[ARM::QPRRegClassID].contains(Reg)) {
@@ -4644,6 +4648,8 @@ bool ARMAsmParser::parseRegisterList(OperandVector &Operands, bool EnforceOrder,
RC = &ARMMCRegisterClasses[ARM::SPRRegClassID];
else if (ARMMCRegisterClasses[ARM::GPRwithAPSRnospRegClassID].contains(Reg))
RC = &ARMMCRegisterClasses[ARM::GPRwithAPSRnospRegClassID];
+ else if (Reg == ARM::VPR)
+ RC = &ARMMCRegisterClasses[ARM::FPWithVPRRegClassID];
else
return Error(RegLoc, "invalid register in register list");
@@ -6335,7 +6341,8 @@ bool ARMAsmParser::parseOperand(OperandVector &Operands, StringRef Mnemonic) {
case AsmToken::LBrac:
return parseMemory(Operands);
case AsmToken::LCurly: {
- bool AllowOutOfBoundReg = Mnemonic == "vlldm" || Mnemonic == "vlstm";
+ bool AllowOutOfBoundReg =
+ Mnemonic == "vlldm" || Mnemonic == "vlstm" || Mnemonic == "vscclrm";
return parseRegisterList(Operands, !Mnemonic.starts_with("clr"), false,
AllowOutOfBoundReg);
}
diff --git a/llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp b/llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
index 93b74905fc59fc..fb08309aa3ab2e 100644
--- a/llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
+++ b/llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
@@ -1528,15 +1528,19 @@ static const uint16_t DPRDecoderTable[] = {
ARM::D28, ARM::D29, ARM::D30, ARM::D31
};
-static DecodeStatus DecodeDPRRegisterClass(MCInst &Inst, unsigned RegNo,
- uint64_t Address,
- const MCDisassembler *Decoder) {
+// Does this instruction/subtarget permit use of registers d16-d31?
+static bool PermitsD32(const MCInst &Inst, const MCDisassembler *Decoder) {
+ if (Inst.getOpcode() == ARM::VSCCLRMD)
+ return true;
const FeatureBitset &featureBits =
((const MCDisassembler*)Decoder)->getSubtargetInfo().getFeatureBits();
+ return featureBits[ARM::FeatureD32];
+}
- bool hasD32 = featureBits[ARM::FeatureD32];
-
- if (RegNo > 31 || (!hasD32 && RegNo > 15))
+static DecodeStatus DecodeDPRRegisterClass(MCInst &Inst, unsigned RegNo,
+ uint64_t Address,
+ const MCDisassembler *Decoder) {
+ if (RegNo > (PermitsD32(Inst, Decoder) ? 31 : 15))
return MCDisassembler::Fail;
unsigned Register = DPRDecoderTable[RegNo];
@@ -1815,10 +1819,11 @@ static DecodeStatus DecodeDPRRegListOperand(MCInst &Inst, unsigned Val,
unsigned regs = fieldFromInstruction(Val, 1, 7);
// In case of unpredictable encoding, tweak the operands.
- if (regs == 0 || regs > 16 || (Vd + regs) > 32) {
- regs = Vd + regs > 32 ? 32 - Vd : regs;
+ unsigned MaxReg = PermitsD32(Inst, Decoder) ? 32 : 16;
+ if (regs == 0 || (Vd + regs) > MaxReg) {
+ regs = Vd + regs > MaxReg ? MaxReg - Vd : regs;
regs = std::max( 1u, regs);
- regs = std::min(16u, regs);
+ regs = std::min(MaxReg, regs);
S = MCDisassembler::SoftFail;
}
@@ -6446,16 +6451,17 @@ static DecodeStatus DecodeVSCCLRM(MCInst &Inst, unsigned Insn, uint64_t Address,
Inst.addOperand(MCOperand::createImm(ARMCC::AL));
Inst.addOperand(MCOperand::createReg(0));
- if (Inst.getOpcode() == ARM::VSCCLRMD) {
- unsigned reglist = (fieldFromInstruction(Insn, 1, 7) << 1) |
- (fieldFromInstruction(Insn, 12, 4) << 8) |
+ unsigned regs = fieldFromInstruction(Insn, 0, 8);
+ if (regs == 0) {
+ // Register list contains only VPR
+ } else if (Inst.getOpcode() == ARM::VSCCLRMD) {
+ unsigned reglist = regs | (fieldFromInstruction(Insn, 12, 4) << 8) |
(fieldFromInstruction(Insn, 22, 1) << 12);
if (!Check(S, DecodeDPRRegListOperand(Inst, reglist, Address, Decoder))) {
return MCDisassembler::Fail;
}
} else {
- unsigned reglist = fieldFromInstruction(Insn, 0, 8) |
- (fieldFromInstruction(Insn, 22, 1) << 8) |
+ unsigned reglist = regs | (fieldFromInstruction(Insn, 22, 1) << 8) |
(fieldFromInstruction(Insn, 12, 4) << 9);
if (!Check(S, DecodeSPRRegListOperand(Inst, reglist, Address, Decoder))) {
return MCDisassembler::Fail;
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp b/llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
index 92427b41f0bb3d..53dad96a00f6de 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
@@ -1743,7 +1743,7 @@ getRegisterListOpValue(const MCInst &MI, unsigned Op,
unsigned Binary = 0;
- if (SPRRegs || DPRRegs) {
+ if (SPRRegs || DPRRegs || Reg == ARM::VPR) {
// VLDM/VSTM/VSCCLRM
unsigned RegNo = CTX.getRegisterInfo()->getEncodingValue(Reg);
unsigned NumRegs = (MI.getNumOperands() - Op) & 0xff;
diff --git a/llvm/test/MC/ARM/vlstm-vlldm-diag.s b/llvm/test/MC/ARM/vlstm-vlldm-diag.s
index b57f535c6a25cf..7aa48b96ff2f69 100644
--- a/llvm/test/MC/ARM/vlstm-vlldm-diag.s
+++ b/llvm/test/MC/ARM/vlstm-vlldm-diag.s
@@ -36,6 +36,14 @@ vlldm r8, {d3 - d31}
// ERR: error: operand must be exactly {d0-d15} (T1) or {d0-d31} (T2)
// ERR-NEXT: vlldm r8, {d3 - d31}
+vlstm r8, {d31}
+// ERR: error: operand must be exactly {d0-d15} (T1) or {d0-d31} (T2)
+// ERR-NEXT: vlstm r8, {d31}
+
+vlldm r8, {d31}
+// ERR: error: operand must be exactly {d0-d15} (T1) or {d0-d31} (T2)
+// ERR-NEXT: vlldm r8, {d31}
+
vlstm r8, {d0 - d35}
// ERR: error: register expected
// ERR-NEXT: vlstm r8, {d0 - d35}
diff --git a/llvm/test/MC/ARM/vscclrm-asm.s b/llvm/test/MC/ARM/vscclrm-asm.s
index 0989b38b07c06e..94d6d3c51dd8da 100644
--- a/llvm/test/MC/ARM/vscclrm-asm.s
+++ b/llvm/test/MC/ARM/vscclrm-asm.s
@@ -35,11 +35,41 @@ it hi
// CHECK: vscclrmhi {s3, s4, s5, s6, s7, s8, s9, s10, s11, s12, s13, s14, s15, s16, s17, s18, s19, s20, s21, s22, s23, s24, s25, s26, s27, s28, s29, s30, s31, vpr} @ encoding: [0xdf,0xec,0x1d,0x1a]
vscclrmhi {s3-s31, vpr}
+// CHECK: vscclrm {vpr} @ encoding: [0x9f,0xec,0x00,0x0a]
+vscclrm {vpr}
+
+// CHECK: vscclrm {d0, d1, d2, d3, d4, d5, d6, d7, d8, d9, d10, d11, d12, d13, d14, d15, d16, d17, d18, d19, d20, d21, d22, d23, d24, d25, d26, d27, d28, d29, d30, d31, vpr} @ encoding: [0x9f,0xec,0x40,0x0b]
+vscclrm {d0-d31, vpr}
+
+// CHECK: vscclrm {d31, vpr} @ encoding: [0xdf,0xec,0x02,0xfb]
+vscclrm {d31, vpr}
+
// ERROR: non-contiguous register range
vscclrm {s0, s3-s4, vpr}
// ERROR: register expected
vscclrm {s32, vpr}
+// ERROR: register expected
+vscclrm {d32, vpr}
+
+// ERROR: register expected
+vscclrm {s31-s32, vpr}
+
+// ERROR: register expected
+vscclrm {d31-d32, vpr}
+
// ERROR: invalid operand for instruction
vscclrm {s0-s1}
+
+// ERROR: register list not in ascending order
+vscclrm {vpr, s0}
+
+// ERROR: register list not in ascending order
+vscclrm {vpr, s31}
+
+// ERROR: register list not in ascending order
+vscclrm {vpr, d0}
+
+// ERROR: register list not in ascending order
+vscclrm {vpr, d31}
diff --git a/llvm/test/MC/Disassembler/ARM/vscclrm.txt b/llvm/test/MC/Disassembler/ARM/vscclrm.txt
index 8a89cfb76e4a45..b7adaaf69d30a3 100644
--- a/llvm/test/MC/Disassembler/ARM/vscclrm.txt
+++ b/llvm/test/MC/Disassembler/ARM/vscclrm.txt
@@ -1,5 +1,7 @@
-# RUN: llvm-mc -disassemble -triple=thumbv8.1m.main-none-eabi -mattr=+8msecext -show-encoding %s 2>&1 | FileCheck %s
-# RUN: llvm-mc -disassemble -triple=thumbv8.1m.main-none-eabi -mattr=+mve.fp,+8msecext -show-encoding %s 2>&1 | FileCheck %s
+# RUN: llvm-mc -disassemble -triple=thumbv8.1m.main-none-eabi -mattr=+8msecext -show-encoding %s 2> %t | FileCheck %s
+# RUN: FileCheck --check-prefix=WARN < %t %s
+# RUN: llvm-mc -disassemble -triple=thumbv8.1m.main-none-eabi -mattr=+mve.fp,+8msecext -show-encoding %s 2> %t | FileCheck %s
+# RUN: FileCheck --check-prefix=WARN < %t %s
[0x9f 0xec 0x04 0x0a]
# CHECK: vscclrm {s0, s1, s2, s3, vpr}
@@ -27,3 +29,36 @@
[0xdf 0xec 0x1d 0x1a]
# CHECK: vscclrmhi {s3, s4, s5, s6, s7, s8, s9, s10, s11, s12, s13, s14, s15, s16, s17, s18, s19, s20, s21, s22, s23, s24, s25, s26, s27, s28, s29, s30, s31, vpr}
+
+# If the list size is zero then we get a list of only vpr, and the Vd register
+# doesn't matter.
+
+[0x9f,0xec,0x00,0x0b]
+# CHECK: vscclrm {vpr} @ encoding: [0x9f,0xec,0x00,0x0b]
+
+[0xdf,0xec,0x00,0xfb]
+# CHECK: vscclrm {vpr} @ encoding: [0x9f,0xec,0x00,0x0b]
+
+[0x9f,0xec,0x00,0x0a]
+# CHECK: vscclrm {vpr} @ encoding: [0x9f,0xec,0x00,0x0a]
+
+[0xdf,0xec,0x00,0xfa]
+# CHECK: vscclrm {vpr} @ encoding: [0x9f,0xec,0x00,0x0a]
+
+# If Vd+size goes past 31 the excess registers are ignored and we get a warning.
+
+[0x9f,0xec,0xfe,0x0b]
+# WARN: [[@LINE-1]]:2: warning: potentially undefined instruction encoding
+# CHECK: vscclrm {d0, d1, d2, d3, d4, d5, d6, d7, d8, d9, d10, d11, d12, d13, d14, d15, d16, d17, d18, d19, d20, d21, d22, d23, d24, d25, d26, d27, d28, d29, d30, d31, vpr} @ encoding: [0x9f,0xec,0x40,0x0b]
+
+[0xdf,0xec,0x04,0xfb]
+# WARN: [[@LINE-1]]:2: warning: potentially undefined instruction encoding
+# CHECK: vscclrm {d31, vpr} @ encoding: [0xdf,0xec,0x02,0xfb]
+
+[0x9f,0xec,0xff,0x0a]
+# WARN: [[@LINE-1]]:2: warning: potentially undefined instruction encoding
+# CHECK: vscclrm {s0, s1, s2, s3, s4, s5, s6, s7, s8, s9, s10, s11, s12, s13, s14, s15, s16, s17, s18, s19, s20, s21, s22, s23, s24, s25, s26, s27, s28, s29, s30, s31, vpr} @ encoding: [0x9f,0xec,0x20,0x0a]
+
+[0xdf,0xec,0x02,0xfa]
+# WARN: [[@LINE-1]]:2: warning: potentially undefined instruction encoding
+# CHECK: vscclrm {s31, vpr} @ encoding: [0xdf,0xec,0x01,0xfa]
|
statham-arm
reviewed
Oct 10, 2024
| [0xdf,0xec,0x00,0xfa] | ||
| # CHECK: vscclrm {vpr} @ encoding: [0x9f,0xec,0x00,0x0a] | ||
|
|
||
| # If Vd+size goes past 31 the excess registers are ignored and we get a warning. |
Collaborator
There was a problem hiding this comment.
I think VSCCLRM is even more unusual than you say.
Reading the ARMARM for the single-precision version of VSCCLRM, the validity criteria are:
topReg = d+regs-1;
if topReg > 63 then UNPREDICTABLE;
if topReg > 31 && topReg[0] == '0' then UNPREDICTABLE;
which suggests that you're supposed to be able to go past s31 into the region past the end of the s-registers where d16-d31 live, and continue clearing d-registers, as long as you don't stop half way through one. Confirmed by the text about <sreglist>: "Registers above S31 are specified by using D registers in the register list."
I think that, for example, [0xdf,0xec,0x07,0xea] ought to disassemble as vscclrm {s29, s30, s31, d16, d17, vpr} with no unpredictability warning: clearing 7 s-register sized pieces starting at s29 means you overrun the end of the s regs by 4, i.e. by 2 d-regs.
statham-arm
approved these changes
Oct 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The register list in vscclrm is unusual in three ways:
Fixing this also incidentally changes a vlldm/vlstm error message: when the first register is in the range d16-d31 we now get the "operand must be exactly..." error instead of "register expected".