Patch [4/4] in a series to add parsing of predicates and properly parse SVE ZIP1/ZIP2 instructions. This patch further improves diagnostic messages for when the SVE feature is not specified.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Hi @sdesmalen,
I have some concerns about this patch and the previous patch in this series as I believe this patch working around how the assembler works, and how the behaviour of the SVE feature is inconsistent with the behaviour of the NEON feature.
The root of the problem as I see it is that AArch64AsmParser::MatchOperandImpl skips any attempt to parse the operands of an instruction when it's associated feature bit is missing. In the no-NEON case, the fallback path can parse and push back the NEON/gpr register or immediate operands. The no-SVE case has no fallback path, so the register operands are instead parsed as immediates.
MatchAndEmitInstruction then attempts to validate the operands of parsed instruction against the corresponding operand classes. The no-NEON case has an instruction selected but that instruction is rejected due to the missing feature bit. The no-SVE case however cannot get a correct failure because the immediates don't correspond to the register classes. This wrong failure then blames the operands rather than the lack of the feature bit.
Consider:
dev:llvmgitsvnbuild$ cat test-neon.s
saddlv h0, v1.8b
zip1 z21.d, z10.d, z21.d
dev:llvmgitsvnbuild$ ./bin/llvm-mc -triple=arm64 < test-neon.s -show-inst-operands -mattr=-neon,-sve
.text
<stdin>:1:3: note: parsed instruction: ['saddlv', <register 72>, <register 121>, '.8b']
saddlv h0, v1.8b
^
<stdin>:1:3: error: instruction requires: neon
saddlv h0, v1.8b
^
<stdin>:2:3: note: parsed instruction: ['zip1', z21.d, z10.d, z21.d]
zip1 z21.d, z10.d, z21.d
^
<stdin>:2:11: error: invalid predicate register.
zip1 z21.d, z10.d, z21.d
^
dev:llvmgitsvnbuild$ ./bin/llvm-mc -triple=arm64 < test-neon.s -show-inst-operands -mattr=+neon,+sve
.text
<stdin>:1:3: note: parsed instruction: ['saddlv', <register 72>, <register 121>, '.8b']
saddlv h0, v1.8b
^
saddlv h0, v1.8b
<stdin>:2:3: note: parsed instruction: ['zip1', <register 265>, <register 254>, <register 265>]
zip1 z21.d, z10.d, z21.d
^
zip1 z21.d, z10.d, z21.dAs you can see, the parse of sve instructions is dependant on the feature bit, but neon case isn't. I would suggest extending AArch64AsmParser::parseRegister to handle the SVE case rather than flipping the SVE bit before attempting to parse an instructions operands.
Thanks.
Comment Actions
Hi @sdardis, thanks for giving this a thorough look. You are right that I've tried to work around how the assembler works and deviated from how Neon operands are parsed, but I have actually done that on purpose.
(Apologies in advance if I am repeating things below)
By default, the 'MatchOperandParserImpl()' function as generated by TableGen does not parse operands if their feature bit is disabled. In contrast, MatchInstructionImpl(), also generated by TableGen, does parse the instruction as if all feature bits have been enabled, so that it can give an appropriate 'requires: <feature>' diagnostic. To me, this mismatch in behaviour sounds like a deficiency of the assembler rather than the desired behaviour (but please correct me if I'm wrong here) and I think that the AArch64AsmParser::parseOperand() function tries to work around that with a custom fallback method.
What I would like for SVE operand parsing is *exactly* what MatchOperandParserImpl() does, namely: Try to parse the possible operands for each instruction with matching mnemonic.
This seems cleaner than reimplementing this functionality in custom code in 'AArch64AsmParser::parseOperand()'. I tried to fix TableGen but found that a number of tests for other targets failed which probably rely on this behaviour, which is why this patch only briefly sets the +sve feature when parsing the operand only to work around the deficiency in MatchOperandParserImpl().
I'm interested to hear your thoughts on how you think the assembler should parse the operands/instructions, as I agree fiddling with the feature bits may not be the desired solution (although it is effective).
Perhaps needless to point out, I think that patch 3/4 (https://reviews.llvm.org/D40362) is a bugfix that is separate from, yet exposed by the way we implemented our assembler.
Comment Actions
Removed fiddling with Feature bits specifically for SVE and changed it so that the AArch64 assembler always parses the possible operands for _all_ instructions matching the mnemonic (enabled by an option to MatchOperandParserImpl()). This improved two other AArch64 assembler tests.
Comment Actions
Thanks Sander. IMO this is a nice improvement to the error messages and not SVE specific. Could you update the title & description? The only potential issue I can think about the same assembler string being enabled by multiple target features. What would happen in that case? The worst thing that can happen is only printing 1 of the target features to enable it?
Also adding Eric & Eli, as they might have some thoughts on this.
| utils/TableGen/AsmMatcherEmitter.cpp | ||
|---|---|---|
| 2800 ↗ | (On Diff #124754) | I think we should clarify here who's responsibility is to print the error message when ParseForAllFeatures = true. Also, if (!ParseForAllFeatures && (AvailableFeatures & it->RequiredFeatures!= it->RequiredFeatures) {? |
Comment Actions
Updated tests to reflect changes from patch [2/4] in this series. Also refactored if-condition.
| utils/TableGen/AsmMatcherEmitter.cpp | ||
|---|---|---|
| 2800 ↗ | (On Diff #124754) | I don't think it is necessary to explain that here, as this code is strictly about parsing of the operands. |
Comment Actions
LGTM, this is an optional straight-forward extension to the AstMatcher, that improves the ASM diagnostics on AArch64 for instructions that require architecture extensions. Could you check if we have similar features & test cases for Armv8.1-a instructions before committing this?
Comment Actions
The Armv8.1-a assembler tests don't seem to include cases to run with the Armv8.1-a features disabled, so I ran some manually and got mostly expected results.
One thing however that is worth pointing out is that for e.g. the 'casp' instructions it will now give a specific error other than 'invalid operand for instruction', because when it tries to parse the operands for 'casp' the tryParseGPRSeqPair method itself emits custom error messages depending on what it expects. Because it uses direct calls to Error(), those will not be prevented by the mechanism implemented by: https://reviews.llvm.org/D40362.
I don't think this is ever much of an issue in practice, but if it will be, then it is possible to create a wrapper function around Error() that checks the feature bits.
Comment Actions
Ok, I think that's fine and we do not have to fix that in this patch. However I think there is value in providing consistent error messages. Please file a bug report if the error message for caps is inconsistent/worse than it could be