[ARM] FIX: change pkhtb custom parsing produce NoMatch rather than Error
#85765
Conversation
This was broken by llvm#83436 as in optional operands meant sometimes the parsePKHImm parser is applied to operands in other positions, which previously produced an error. Now this instead fails the parse. However this unfortunately means the default parsing happens which combines the shift and the base operator which causes the diagnostics to be non-sense. To fix this I had to add a special case diagnostic which is unfortunate. Ideally these two methods for handling operands should be unified and the TableGen matching mechanisms should be used for validaing parsed shifts rather than special cases in parsing.
|
@llvm/pr-subscribers-backend-arm Author: Alfie Richards (AlfieRichardsArm) ChangesThis was broken by #83436 as in optional operands meant when the CC operand is provided the This patch changes To fix this I added a special case diagnostic to this default parsing which is little unfortunate. Ideally these two methods for handling operands should be unified and the TableGen matching mechanisms should be used for validaing parsed shifts rather than special cases in parsing. Full diff: https://github.com/llvm/llvm-project/pull/85765.diff 3 Files Affected:
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index 026d4ca06e87b9..e21310c53f45a0 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -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,
@@ -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.
diff --git a/llvm/test/MC/ARM/basic-arm-instructions.s b/llvm/test/MC/ARM/basic-arm-instructions.s
index 055f3ce316153d..84a7cf52fa30e0 100644
--- a/llvm/test/MC/ARM/basic-arm-instructions.s
+++ b/llvm/test/MC/ARM/basic-arm-instructions.s
@@ -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]
@@ -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
diff --git a/llvm/test/MC/ARM/diagnostics.s b/llvm/test/MC/ARM/diagnostics.s
index fa23a7da1e4048..b6d011e5bdef8f 100644
--- a/llvm/test/MC/ARM/diagnostics.s
+++ b/llvm/test/MC/ARM/diagnostics.s
@@ -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
@ 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: ^
|
|
@llvm/pr-subscribers-mc Author: Alfie Richards (AlfieRichardsArm) ChangesThis was broken by #83436 as in optional operands meant when the CC operand is provided the This patch changes To fix this I added a special case diagnostic to this default parsing which is little unfortunate. Ideally these two methods for handling operands should be unified and the TableGen matching mechanisms should be used for validaing parsed shifts rather than special cases in parsing. Full diff: https://github.com/llvm/llvm-project/pull/85765.diff 3 Files Affected:
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index 026d4ca06e87b9..e21310c53f45a0 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -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,
@@ -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.
diff --git a/llvm/test/MC/ARM/basic-arm-instructions.s b/llvm/test/MC/ARM/basic-arm-instructions.s
index 055f3ce316153d..84a7cf52fa30e0 100644
--- a/llvm/test/MC/ARM/basic-arm-instructions.s
+++ b/llvm/test/MC/ARM/basic-arm-instructions.s
@@ -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]
@@ -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
diff --git a/llvm/test/MC/ARM/diagnostics.s b/llvm/test/MC/ARM/diagnostics.s
index fa23a7da1e4048..b6d011e5bdef8f 100644
--- a/llvm/test/MC/ARM/diagnostics.s
+++ b/llvm/test/MC/ARM/diagnostics.s
@@ -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
@ 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: ^
|
|
Note, this fixes a regression introduced in #83436 brought up here: |
|
|
There was a problem hiding this comment.
+1, thanks, this does fix my build issue. I can't say I fully understand the code changes here (without spending another 15 minutes on reading up on what's going on here), but it does unbreak things.
I'm leaving it unapproved still, if someone else from Arm wants to have a look and give it a proper review, otherwise I guess this can go in later today.
llvm/test/MC/ARM/diagnostics.s
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
@jthackray Just pushed a change that achieves the same thing a much cleaner way. I'm much happier with this change! |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Yes, good improvement, and removes the error string change to the other testcase. |
|
I went ahead and landed this now, to unbreak my builds. I tried to update the commit message according to the solution chosen in the end. (If unhappy with the way I phrased it, feel free to revert and recommit with a more correct message.) |
This was broken by #83436 as in optional operands meant when the CC operand is provided the
parsePKHImmparser is applied to register operands, which previously erroneously produced an error.This patch changes
parsePKHImmto provideNoMatchinstead. However this unfortunately means the default parsing happens which combines the shift and the base operator which causes the diagnostics to be nonsense.To fix this I added a special case diagnostic to this default parsing which is little unfortunate. Ideally these two methods for handling operands should be unified and the TableGen matching mechanisms should be used for validaing parsed shifts rather than special cases in parsing.