[SPARC][IAS] Reject unknown/unavailable mnemonics early in ParseInstruction #96021
Conversation
Created using spr 1.3.5 [skip ci]
Created using spr 1.3.5
|
@llvm/pr-subscribers-backend-sparc @llvm/pr-subscribers-mc Author: Koakuma (koachan) ChangesThis enables Full diff: https://github.com/llvm/llvm-project/pull/96021.diff 3 Files Affected:
diff --git a/llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp b/llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
index 3d8637bb8c359..34531fff31e8e 100644
--- a/llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
+++ b/llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
@@ -1189,7 +1189,8 @@ ParseStatus SparcAsmParser::parseCallTarget(OperandVector &Operands) {
ParseStatus SparcAsmParser::parseOperand(OperandVector &Operands,
StringRef Mnemonic) {
- ParseStatus Res = MatchOperandParserImpl(Operands, Mnemonic);
+ ParseStatus Res =
+ MatchOperandParserImpl(Operands, Mnemonic, /*ParseForAllFeatures=*/true);
// If there wasn't a custom match, try the generic matcher below. Otherwise,
// there was a match, but an error occurred, in which case, just return that
diff --git a/llvm/test/MC/Sparc/sparc-asm-errors.s b/llvm/test/MC/Sparc/sparc-asm-errors.s
index 780f4e7fad787..2b3a2eb2bfae6 100644
--- a/llvm/test/MC/Sparc/sparc-asm-errors.s
+++ b/llvm/test/MC/Sparc/sparc-asm-errors.s
@@ -11,7 +11,7 @@
! V9: unknown membar tag
membar #BadTag
- ! V8: instruction requires a CPU feature not currently enabled
+ ! V8: unexpected token
! V9: invalid membar mask number
membar -127
diff --git a/llvm/test/MC/Sparc/sparcv9-instructions.s b/llvm/test/MC/Sparc/sparcv9-instructions.s
index a7761c10c509b..68ae2ac5c98a4 100644
--- a/llvm/test/MC/Sparc/sparcv9-instructions.s
+++ b/llvm/test/MC/Sparc/sparcv9-instructions.s
@@ -537,112 +537,112 @@
! V9: stxa %g0, [%g2+%i5] #ASI_SNF ! encoding: [0xc0,0xf0,0x90,0x7d]
stxa %g0, [%g2 + %i5] #ASI_SNF
- ! V8: error: invalid operand for instruction
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], 1
! V9: prefetch [%i1+3968], #one_read ! encoding: [0xc3,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], 1
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #n_reads
! V9: prefetch [%i1+3968], #n_reads ! encoding: [0xc1,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #n_reads
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #one_read
! V9: prefetch [%i1+3968], #one_read ! encoding: [0xc3,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #one_read
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #n_writes
! V9: prefetch [%i1+3968], #n_writes ! encoding: [0xc5,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #n_writes
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #one_write
! V9: prefetch [%i1+3968], #one_write ! encoding: [0xc7,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #one_write
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #page
! V9: prefetch [%i1+3968], #page ! encoding: [0xc9,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #page
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #unified
! V9: prefetch [%i1+3968], #unified ! encoding: [0xe3,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #unified
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #n_reads_strong
! V9: prefetch [%i1+3968], #n_reads_strong ! encoding: [0xe9,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #n_reads_strong
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #one_read_strong
! V9: prefetch [%i1+3968], #one_read_strong ! encoding: [0xeb,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #one_read_strong
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #n_writes_strong
! V9: prefetch [%i1+3968], #n_writes_strong ! encoding: [0xed,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #n_writes_strong
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + 0xf80 ], #one_write_strong
! V9: prefetch [%i1+3968], #one_write_strong ! encoding: [0xef,0x6e,0x6f,0x80]
prefetch [ %i1 + 0xf80 ], #one_write_strong
- ! V8: error: invalid operand for instruction
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], 1
! V9: prefetch [%i1+%i2], #one_read ! encoding: [0xc3,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], 1
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #n_reads
! V9: prefetch [%i1+%i2], #n_reads ! encoding: [0xc1,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #n_reads
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #one_read
! V9: prefetch [%i1+%i2], #one_read ! encoding: [0xc3,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #one_read
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #n_writes
! V9: prefetch [%i1+%i2], #n_writes ! encoding: [0xc5,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #n_writes
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #one_write
! V9: prefetch [%i1+%i2], #one_write ! encoding: [0xc7,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #one_write
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #page
! V9: prefetch [%i1+%i2], #page ! encoding: [0xc9,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #page
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #unified
! V9: prefetch [%i1+%i2], #unified ! encoding: [0xe3,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #unified
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #n_reads_strong
! V9: prefetch [%i1+%i2], #n_reads_strong ! encoding: [0xe9,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #n_reads_strong
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #one_read_strong
! V9: prefetch [%i1+%i2], #one_read_strong ! encoding: [0xeb,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #one_read_strong
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #n_writes_strong
! V9: prefetch [%i1+%i2], #n_writes_strong ! encoding: [0xed,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #n_writes_strong
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetch [ %i1 + %i2 ], #one_write_strong
! V9: prefetch [%i1+%i2], #one_write_strong ! encoding: [0xef,0x6e,0x40,0x1a]
prefetch [ %i1 + %i2 ], #one_write_strong
@@ -657,22 +657,22 @@
! V9: prefetcha [%i1+3968] %asi, #one_read ! encoding: [0xc3,0xee,0x6f,0x80]
prefetcha [ %i1 + 0xf80 ] %asi, #one_read
- ! V8: error: invalid operand for instruction
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetcha [ %i1 + %i2 ] #ASI_SNF, 1
! V9: prefetcha [%i1+%i2] #ASI_SNF, #one_read ! encoding: [0xc3,0xee,0x50,0x7a]
prefetcha [ %i1 + %i2 ] #ASI_SNF, 1
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetcha [ %i1 + %i2 ] #ASI_SNF, #one_read
! V9: prefetcha [%i1+%i2] #ASI_SNF, #one_read ! encoding: [0xc3,0xee,0x50,0x7a]
prefetcha [ %i1 + %i2 ] #ASI_SNF, #one_read
- ! V8: error: invalid operand for instruction
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetcha [ %i1 + %i2 ] 131, 1
! V9: prefetcha [%i1+%i2] #ASI_SNF, #one_read ! encoding: [0xc3,0xee,0x50,0x7a]
prefetcha [ %i1 + %i2 ] 131, 1
- ! V8: error: unexpected token
+ ! V8: error: instruction requires a CPU feature not currently enabled
! V8-NEXT: prefetcha [ %i1 + %i2 ] 131, #one_read
! V9: prefetcha [%i1+%i2] #ASI_SNF, #one_read ! encoding: [0xc3,0xee,0x50,0x7a]
prefetcha [ %i1 + %i2 ] 131, #one_read
|
| @@ -11,7 +11,7 @@ | |||
| ! V9: unknown membar tag | |||
| membar #BadTag | |||
|
|
|||
| ! V8: instruction requires a CPU feature not currently enabled | |||
| ! V8: unexpected token | |||
| ! V9: invalid membar mask number | |||
| membar -127 | |||
There was a problem hiding this comment.
The membar error messages seems to regress, seems like it prioritizes the parse error over the fact that membar is not available on V8? Am I missing something in the patch?
There was a problem hiding this comment.
There are many places where ParseStatus is not handled correctly.
In this case, it is
if (!parseOperand(Operands, Name).isSuccess()) {
SMLoc Loc = getLexer().getLoc();
return Error(Loc, "unexpected token");
}
It should propagate the error returned by parseOperand instead of issuing another one.
Something like:
ParseStatus R = parseOperand(Operands, Name);
if (R.isFailure())
return true;
if (R.isNoMatch())
return TokError( "unexpected token");
// success, proceed to the next operand
or
ParseStatus R = parseOperand(Operands, Name);
if (R.isNoMatch())
return TokError( "unexpected token");
if (!R.isSuccess())
return R;
// success, proceed to the next operand
There was a problem hiding this comment.
It appears changing this line reveals other bugs. It may be worth making this change separately, before enabling ParseForAllFeatures.
Created using spr 1.3.5 [skip ci]
Created using spr 1.3.5
Created using spr 1.3.5
koachan
changed the title
ParseForAllFeatures in MatchOperandParserImpl
|
Hmmm, so, change of approach - I decided to follow what the MIPS backend does and validate all mnemonics early in ParseInstruction, before any operand parsing is done. The misuse of ParseStatus returns still needs to be addressed, but at least I think with this approach it should no longer be a blocker for this particular issue? |
…vious Created using spr 1.3.5
There was a problem hiding this comment.
The downside of this approach is that you lose the ability to report the missing features (unless you implement it yourself, which would be a code duplication), that would allow to produce better diagnostics like "instruction requires: LeonCASA".
Also, sometimes a mnemonic is valid in both modes, but certain operand combinations are only allowed in V9, e.g. swapa with r+i addressing mode. In this case you still rely on MatchOperandParserImpl & MatchInstructionImpl, which can produce different / broken diagnostics.
I'd still vote for ParseForAllFeatures, but I don't insist since people are unlikely to see these diagnostics anyway.
|
Merging this for now, but to clarify a little:
It seems that there are no autogenerated and/or common functions that can report what is exactly the missing feature. A quick look at the other backends also shows that the ones that do support reporting that implements it by themselves (unless I'm missing something here?).
Yeah, this is what is bugging me too.
In the end the error that will go out is one from the operand check, instead of the mnemonic check - since IAS haven't gotten far enough to reach MatchInstructionImpl yet - unless there is a way to defer the error from the operand check until just after MatchInstructionImpl is done (is it possible?). On cases like [1] MatchOperandParserImpl return value does not differentiate between unavailable mnemonic, unknown mnemonic, or not being able to find a custom parser, so in all three cases parseOperand will just fall through to the generic matcher, so at least in this particular case there is no difference between ParseForAllFeatures being enabled or not. |
koachan
changed the base branch from
users/koachan/spr/main.sparcias-enable-parseforallfeatures-in-matchoperandparserimpl
to
main
koachan
deleted the
users/koachan/spr/sparcias-enable-parseforallfeatures-in-matchoperandparserimpl
branch
…uction Validate and reject any unknown or unavailable instruction mnemonics early in ParseInstruction, before any operand parsing is performed. Some operands (mainly memory ones) can be parsed slightly differently in V8 and V9 assembly language, so by rejecting unknown or unavailable instructions early we can prevent the error message from being shadowed by the one raised during operand parsing. As a side effect this also allows us to tell unknown and unavailable mnemonics apart, and issue a suggestion in appropriate cases. This is based on the approach taken by the MIPS backend. Reviewers: brad0, rorth, s-barannikov, jrtc27 Reviewed By: s-barannikov Pull Request: llvm#96021
Validate and reject any unknown or unavailable instruction mnemonics early
in ParseInstruction, before any operand parsing is performed. Some operands
(mainly memory ones) can be parsed slightly differently in V8 and V9
assembly language, so by rejecting unknown or unavailable instructions early
we can prevent the error message from being shadowed by the one raised during
operand parsing.
As a side effect this also allows us to tell unknown and unavailable
mnemonics apart, and issue a suggestion in appropriate cases.
This is based on the approach taken by the MIPS backend.