[AArch64][MC] Improve isSImm Diagnostic Fidelity
#127255
Conversation
With the change in llvm#126653, it became clear that there was an implicit coercion from DiagnosticPredicate bool in `isSimm`, which is then undone in the TableGen'd code that uses the predicate, converting from a bool back to a DiagnosticPredicate. This roundtrip conversion loses information - if `isSImmScaled` returns `DiagnosticPredicate::NoMatch` then it is converted to `DiagnosticPredicate::NearMatch`, which both will cause an error, but have different behaviour for which diagnostic is used. This change removes the boolean conversions and directly returns the DiagnosticPredicate. This causes some changes to error messages, most of which are that when a register provided but a specific immediate was expected, now the error is "invalid operand for instruction" and not an indication that the immediate was out of range. The only change not like this is to LDR (Register), which seems to just choose a different immediate-related diagnostic when a 64-bit register is used - which was equally inaccurate as the previous error. This is because there are many other places in the parser that do not (yet?) use DiagnosticPredicate, in this case the issue seems to be `isUImm12Offset`, which still returns a `bool`.
|
@llvm/pr-subscribers-mc Author: Sam Elliott (lenary) ChangesWith the change in #126653, it became clear that there was an implicit coercion from DiagnosticPredicate bool in This roundtrip conversion loses information - if This change removes the boolean conversions and directly returns the DiagnosticPredicate. This causes some changes to error messages, most of which are that when a register provided but a specific immediate was expected, now the error is "invalid operand for instruction" and not an indication that the immediate was out of range. The only change not like this is to LDR (Register), which seems to just choose a different immediate-related diagnostic when a 64-bit register is used - which was equally inaccurate as the previous diagnostic. This is because there are many other places in the parser that do not (yet?) use DiagnosticPredicate, in this case the issue seems to be Full diff: https://github.com/llvm/llvm-project/pull/127255.diff 9 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
index 11fb746042e98..f2d206b864f35 100644
--- a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
+++ b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
@@ -814,8 +814,8 @@ class AArch64Operand : public MCParsedAsmOperand {
return (Val >= 0 && Val < 64);
}
- template <int Width> bool isSImm() const {
- return bool(isSImmScaled<Width, 1>());
+ template <int Width> DiagnosticPredicate isSImm() const {
+ return isSImmScaled<Width, 1>();
}
template <int Bits, int Scale> DiagnosticPredicate isSImmScaled() const {
diff --git a/llvm/test/MC/AArch64/SME/addspl-diagnostics.s b/llvm/test/MC/AArch64/SME/addspl-diagnostics.s
index 02b8f6ca765aa..96e926e327b36 100644
--- a/llvm/test/MC/AArch64/SME/addspl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SME/addspl-diagnostics.s
@@ -8,6 +8,6 @@ addspl x19, x14, #32
// addspl requires an immediate, not a register.
addspl x19, x14, x15
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: addspl x19, x14, x15
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SME/addsvl-diagnostics.s b/llvm/test/MC/AArch64/SME/addsvl-diagnostics.s
index d420894bc174c..0a3867952fc25 100644
--- a/llvm/test/MC/AArch64/SME/addsvl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SME/addsvl-diagnostics.s
@@ -8,6 +8,6 @@ addsvl x3, x5, #32
// addsvl requires an immediate, not a register.
addsvl x3, x5, x6
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: addsvl x3, x5, x6
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SME/rdsvl-diagnostics.s b/llvm/test/MC/AArch64/SME/rdsvl-diagnostics.s
index 0ea7d9beaef3b..8e5e85b089102 100644
--- a/llvm/test/MC/AArch64/SME/rdsvl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SME/rdsvl-diagnostics.s
@@ -8,6 +8,6 @@ rdsvl x9, #32
// rdsvl requires an immediate, not a register.
rdsvl x9, x10
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: rdsvl x9, x10
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SVE/addpl-diagnostics.s b/llvm/test/MC/AArch64/SVE/addpl-diagnostics.s
index eb5a80aa4ea9f..266d58abdca33 100644
--- a/llvm/test/MC/AArch64/SVE/addpl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SVE/addpl-diagnostics.s
@@ -8,6 +8,6 @@ addpl x19, x14, #32
// addpl requires an immediate, not a register.
addpl x19, x14, x15
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: addpl x19, x14, x15
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SVE/addvl-diagnostics.s b/llvm/test/MC/AArch64/SVE/addvl-diagnostics.s
index 6c04176d0bd1b..1dc540f501e43 100644
--- a/llvm/test/MC/AArch64/SVE/addvl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SVE/addvl-diagnostics.s
@@ -8,6 +8,6 @@ addvl x3, x5, #32
// addvl requires an immediate, not a register.
addvl x3, x5, x6
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: addvl x3, x5, x6
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SVE/index-diagnostics.s b/llvm/test/MC/AArch64/SVE/index-diagnostics.s
index 3b2a4aa656fd3..2600089cd01c0 100644
--- a/llvm/test/MC/AArch64/SVE/index-diagnostics.s
+++ b/llvm/test/MC/AArch64/SVE/index-diagnostics.s
@@ -48,12 +48,12 @@ index z17.d, x9, #16
// Invalid register
index z17.s, x9, w7
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-16, 15].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: index z17.s, x9, w7
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
index z17.d, w9, w7
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-16, 15].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: index z17.d, w9, w7
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SVE/rdvl-diagnostics.s b/llvm/test/MC/AArch64/SVE/rdvl-diagnostics.s
index 0af37a60e38b0..e78b5f1292bd4 100644
--- a/llvm/test/MC/AArch64/SVE/rdvl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SVE/rdvl-diagnostics.s
@@ -8,6 +8,6 @@ rdvl x9, #32
// rdvl requires an immediate, not a register.
rdvl x9, x10
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: rdvl x9, x10
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/arm64-diags.s b/llvm/test/MC/AArch64/arm64-diags.s
index 591ff64eb3389..5766791748ee3 100644
--- a/llvm/test/MC/AArch64/arm64-diags.s
+++ b/llvm/test/MC/AArch64/arm64-diags.s
@@ -121,7 +121,7 @@ ldr q1, [x3, w3, sxtw #1]
; CHECK-ERRORS: error: expected 'uxtw' or 'sxtw' with optional shift of #0 or #3
; CHECK-ERRORS: str d1, [x3, w3, sxtx #3]
; CHECK-ERRORS: ^
-; CHECK-ERRORS: error: index must be an integer in range [-256, 255].
+; CHECK-ERRORS: error: index must be a multiple of 4 in range [0, 16380].
; CHECK-ERRORS: ldr s1, [x3, d3, sxtx #2]
; CHECK-ERRORS: ^
|
|
@llvm/pr-subscribers-backend-aarch64 Author: Sam Elliott (lenary) ChangesWith the change in #126653, it became clear that there was an implicit coercion from DiagnosticPredicate bool in This roundtrip conversion loses information - if This change removes the boolean conversions and directly returns the DiagnosticPredicate. This causes some changes to error messages, most of which are that when a register provided but a specific immediate was expected, now the error is "invalid operand for instruction" and not an indication that the immediate was out of range. The only change not like this is to LDR (Register), which seems to just choose a different immediate-related diagnostic when a 64-bit register is used - which was equally inaccurate as the previous diagnostic. This is because there are many other places in the parser that do not (yet?) use DiagnosticPredicate, in this case the issue seems to be Full diff: https://github.com/llvm/llvm-project/pull/127255.diff 9 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
index 11fb746042e98..f2d206b864f35 100644
--- a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
+++ b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
@@ -814,8 +814,8 @@ class AArch64Operand : public MCParsedAsmOperand {
return (Val >= 0 && Val < 64);
}
- template <int Width> bool isSImm() const {
- return bool(isSImmScaled<Width, 1>());
+ template <int Width> DiagnosticPredicate isSImm() const {
+ return isSImmScaled<Width, 1>();
}
template <int Bits, int Scale> DiagnosticPredicate isSImmScaled() const {
diff --git a/llvm/test/MC/AArch64/SME/addspl-diagnostics.s b/llvm/test/MC/AArch64/SME/addspl-diagnostics.s
index 02b8f6ca765aa..96e926e327b36 100644
--- a/llvm/test/MC/AArch64/SME/addspl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SME/addspl-diagnostics.s
@@ -8,6 +8,6 @@ addspl x19, x14, #32
// addspl requires an immediate, not a register.
addspl x19, x14, x15
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: addspl x19, x14, x15
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SME/addsvl-diagnostics.s b/llvm/test/MC/AArch64/SME/addsvl-diagnostics.s
index d420894bc174c..0a3867952fc25 100644
--- a/llvm/test/MC/AArch64/SME/addsvl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SME/addsvl-diagnostics.s
@@ -8,6 +8,6 @@ addsvl x3, x5, #32
// addsvl requires an immediate, not a register.
addsvl x3, x5, x6
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: addsvl x3, x5, x6
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SME/rdsvl-diagnostics.s b/llvm/test/MC/AArch64/SME/rdsvl-diagnostics.s
index 0ea7d9beaef3b..8e5e85b089102 100644
--- a/llvm/test/MC/AArch64/SME/rdsvl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SME/rdsvl-diagnostics.s
@@ -8,6 +8,6 @@ rdsvl x9, #32
// rdsvl requires an immediate, not a register.
rdsvl x9, x10
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: rdsvl x9, x10
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SVE/addpl-diagnostics.s b/llvm/test/MC/AArch64/SVE/addpl-diagnostics.s
index eb5a80aa4ea9f..266d58abdca33 100644
--- a/llvm/test/MC/AArch64/SVE/addpl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SVE/addpl-diagnostics.s
@@ -8,6 +8,6 @@ addpl x19, x14, #32
// addpl requires an immediate, not a register.
addpl x19, x14, x15
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: addpl x19, x14, x15
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SVE/addvl-diagnostics.s b/llvm/test/MC/AArch64/SVE/addvl-diagnostics.s
index 6c04176d0bd1b..1dc540f501e43 100644
--- a/llvm/test/MC/AArch64/SVE/addvl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SVE/addvl-diagnostics.s
@@ -8,6 +8,6 @@ addvl x3, x5, #32
// addvl requires an immediate, not a register.
addvl x3, x5, x6
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: addvl x3, x5, x6
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SVE/index-diagnostics.s b/llvm/test/MC/AArch64/SVE/index-diagnostics.s
index 3b2a4aa656fd3..2600089cd01c0 100644
--- a/llvm/test/MC/AArch64/SVE/index-diagnostics.s
+++ b/llvm/test/MC/AArch64/SVE/index-diagnostics.s
@@ -48,12 +48,12 @@ index z17.d, x9, #16
// Invalid register
index z17.s, x9, w7
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-16, 15].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: index z17.s, x9, w7
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
index z17.d, w9, w7
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-16, 15].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: index z17.d, w9, w7
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/SVE/rdvl-diagnostics.s b/llvm/test/MC/AArch64/SVE/rdvl-diagnostics.s
index 0af37a60e38b0..e78b5f1292bd4 100644
--- a/llvm/test/MC/AArch64/SVE/rdvl-diagnostics.s
+++ b/llvm/test/MC/AArch64/SVE/rdvl-diagnostics.s
@@ -8,6 +8,6 @@ rdvl x9, #32
// rdvl requires an immediate, not a register.
rdvl x9, x10
-// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: index must be an integer in range [-32, 31].
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
// CHECK-NEXT: rdvl x9, x10
// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
diff --git a/llvm/test/MC/AArch64/arm64-diags.s b/llvm/test/MC/AArch64/arm64-diags.s
index 591ff64eb3389..5766791748ee3 100644
--- a/llvm/test/MC/AArch64/arm64-diags.s
+++ b/llvm/test/MC/AArch64/arm64-diags.s
@@ -121,7 +121,7 @@ ldr q1, [x3, w3, sxtw #1]
; CHECK-ERRORS: error: expected 'uxtw' or 'sxtw' with optional shift of #0 or #3
; CHECK-ERRORS: str d1, [x3, w3, sxtx #3]
; CHECK-ERRORS: ^
-; CHECK-ERRORS: error: index must be an integer in range [-256, 255].
+; CHECK-ERRORS: error: index must be a multiple of 4 in range [0, 16380].
; CHECK-ERRORS: ldr s1, [x3, d3, sxtx #2]
; CHECK-ERRORS: ^
|
|
ping |
|
From the tests, it looks like this is making the diagnostics worse. Is there some future improvement which this is needed to unblock, or could we change |
If you look at the full test files, the implementer of the diagnostics made a specific choice to give "index must be in range [...]" errors only when an index/immediate has been provided - and to just give "invalid operand ..." when the user provided something that's not an immediate (in these tests, a register). This is how the I'm not bothered either way, this is just something I noticed. I'm not spending several months refactoring the AArch64 assembler error messages, I only got here because I wanted to start using DiagnosticPredicates for RISC-V. |
With the change in #126653, it became clear that there was an implicit coercion from DiagnosticPredicate bool in
isSimm, which is then undone in the TableGen'd code that uses the predicate, converting from a bool back to a DiagnosticPredicate.This roundtrip conversion loses information - if
isSImmScaledreturnsDiagnosticPredicate::NoMatchthen it is converted toDiagnosticPredicate::NearMatch, which both will cause an error, but have different behaviour for which diagnostic is used.This change removes the boolean conversions and directly returns the DiagnosticPredicate. This causes some changes to error messages, most of which are that when a register provided but a specific immediate was expected, now the error is "invalid operand for instruction" and not an indication that the immediate was out of range.
The only change not like this is to LDR (Register), which seems to just choose a different immediate-related diagnostic when a 64-bit register is used - which was equally inaccurate as the previous diagnostic. This is because there are many other places in the parser that do not (yet?) use DiagnosticPredicate, in this case the issue seems to be
isUImm12Offset, which still returns abool.