This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sdesmalen on Nov 22 2017, 9:32 AM.

Details

Summary

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.

Diff Detail

Event Timeline

sdesmalen created this revision.Nov 22 2017, 9:32 AM

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.d

As 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.

Addendum: that example was with the first two patches in this series applied.

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.

sdesmalen updated this revision to Diff 124754.Nov 29 2017, 8:01 AM

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.

fhahn edited edge metadata.

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.

fhahn added inline comments.Nov 29 2017, 9:09 AM
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) {?

sdesmalen updated this revision to Diff 127126.Dec 15 2017, 7:13 AM

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.

fhahn accepted this revision.Dec 15 2017, 12:32 PM

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?

This revision is now accepted and ready to land.Dec 15 2017, 12:32 PM

Could you check if we have similar features & test cases for Armv8.1-a instructions before committing this?

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.

fhahn added a comment.Dec 18 2017, 7:36 AM

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

sdesmalen closed this revision.Dec 18 2017, 8:49 AM