Skip to content

[ARM] FIX: change pkhtb custom parsing produce NoMatch rather than Error #85765

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
mstorsjo merged 7 commits into from
Mar 19, 2024
Merged

[ARM] FIX: change pkhtb custom parsing produce NoMatch rather than Error #85765

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
61ee60c
[ARM] Fix PKHImm parsing.
AlfieRichardsArm Mar 19, 2024
ad4b0ab
Merge branch 'main' into ffmpeg-pkhtb-fix
AlfieRichardsArm Mar 19, 2024
b9957c4
Nit fix
AlfieRichardsArm Mar 19, 2024
b33f00d
Much neater way to achieve same result. Avoids changing the diagnosti…
AlfieRichardsArm Mar 19, 2024
73eca78
Formatting
AlfieRichardsArm Mar 19, 2024
c027162
Add back `separate`
AlfieRichardsArm Mar 19, 2024
dc503c2
Formatting
AlfieRichardsArm Mar 19, 2024
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
21 changes: 19 additions & 2 deletions llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4318,6 +4318,23 @@ int ARMAsmParser::tryParseShiftRegister(OperandVector &Operands) {
}
}

auto MnemonicOp = static_cast<ARMOperand &>(*Operands[0]);
// These instructions handle shift operands as seperate operators to the
// register. In these cases doing the default conversion makes nonsense
// diagnostics.
// FIXME: Unify the different methods for handling shift operators
// and use TableGen matching mechanisms to do the validation.
static const DenseSet<StringRef> NoDefaultConvertSet{"pkhbt", "pkhtb"};
bool ShouldDefaultConvert =
(MnemonicOp.isToken() &&
!NoDefaultConvertSet.contains(MnemonicOp.getToken()));
if (!ShouldDefaultConvert) {
// If we get to this point the parsing for the shift operator in
// parsePKHLSLImm has failed. So we can generate a diagnostic here.
Error(S, "shift operator is malformed for this instruction");
return -1;
}

if (ShiftReg && ShiftTy != ARM_AM::rrx)
Operands.push_back(ARMOperand::CreateShiftedRegister(ShiftTy, SrcReg,
ShiftReg, Imm,
Expand Down Expand Up @@ -5289,12 +5306,12 @@ ParseStatus ARMAsmParser::parsePKHImm(OperandVector &Operands, StringRef Op,
MCAsmParser &Parser = getParser();
const AsmToken &Tok = Parser.getTok();
if (Tok.isNot(AsmToken::Identifier))
return Error(Parser.getTok().getLoc(), Op + " operand expected.");
return ParseStatus::NoMatch;
StringRef ShiftName = Tok.getString();
std::string LowerOp = Op.lower();
std::string UpperOp = Op.upper();
if (ShiftName != LowerOp && ShiftName != UpperOp)
return Error(Parser.getTok().getLoc(), Op + " operand expected.");
return ParseStatus::NoMatch;
Parser.Lex(); // Eat shift type token.

// There must be a '#' and a shift amount.
Expand Down
4 changes: 4 additions & 0 deletions llvm/test/MC/ARM/basic-arm-instructions.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1774,6 +1774,9 @@ Lforward:
pkhtb r2, r2, r3, asr #31
pkhtb r2, r2, r3, asr #15

it ne
pkhtbne r2, r2, r3, asr #15

@ CHECK: pkhbt r2, r2, r3 @ encoding: [0x13,0x20,0x82,0xe6]
@ CHECK: pkhbt r2, r2, r3, lsl #31 @ encoding: [0x93,0x2f,0x82,0xe6]
@ CHECK: pkhbt r2, r2, r3 @ encoding: [0x13,0x20,0x82,0xe6]
Expand All @@ -1782,6 +1785,7 @@ Lforward:
@ CHECK: pkhbt r2, r3, r2 @ encoding: [0x12,0x20,0x83,0xe6]
@ CHECK: pkhtb r2, r2, r3, asr #31 @ encoding: [0xd3,0x2f,0x82,0xe6]
@ CHECK: pkhtb r2, r2, r3, asr #15 @ encoding: [0xd3,0x27,0x82,0xe6]
@ CHECK: pkhtbne r2, r2, r3, asr #15 @ encoding: [0xd3,0x27,0x82,0x16]

@------------------------------------------------------------------------------
@ FIXME: PLD
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/MC/ARM/diagnostics.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,10 @@
@ CHECK-ERRORS: error: immediate value out of range
@ CHECK-ERRORS: pkhtb r2, r2, r3, asr #33
@ CHECK-ERRORS: ^
@ CHECK-ERRORS: error: lsl operand expected.
@ CHECK-ERRORS: error: shift operator is malformed for this instruction
Copy link
Copy Markdown
Contributor

@jthackray jthackray Mar 19, 2024

Choose a reason for hiding this comment

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

I can see you've added a new testcase in basic-arm-instructions.s for the specific failure mentioned in the bug report, but is this change because you've added the new error message? Presumably this is now reached instead, hence the change in this testcase?

Copy link
Copy Markdown
Contributor Author

@AlfieRichardsArm AlfieRichardsArm Mar 19, 2024

Choose a reason for hiding this comment

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

Yeah, the old mechanism for the diagnostic was throwing an error when parsing failed.
Unfortunately with the optional operand change the parsing gets applied to more operands and is expected to return NoMatch when there genuinely isn't a match.

Actually, I still handle this better. Let me fix this with a better diagnostic really quickly.

@ CHECK-ERRORS: pkhbt r2, r2, r3, asr #3
@ CHECK-ERRORS: ^
@ CHECK-ERRORS: error: asr operand expected.
@ CHECK-ERRORS: error: shift operator is malformed for this instruction
@ CHECK-ERRORS: pkhtb r2, r2, r3, lsl #3
@ CHECK-ERRORS: ^

Expand Down