[PATCH] D40363: [AArch64][SVE] Asm: Improve diagnostics further when +sve is not specified

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

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.


https://reviews.llvm.org/D40363