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

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

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.


https://reviews.llvm.org/D40363