[PATCH] D31530: [ARM] Use new assembler diags for ARM

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added inline comments.


================
Comment at: test/MC/ARM/arm-branch-errors.s:14
+@ CHECK: note: for one encoding: instruction requires: thumb
+@ CHECK: note: for one encoding: invalid operand for instruction
+@ CHECK: error: invalid instruction, multiple near-miss encodings found
----------------
rengolin wrote:
> This is a bit hard to understand... I'm not sure what you mean by "for one encoding".
> 
> Of course, the best message here would be "address needs to be 4-byte aligned on ARM", I'm not sure how we could get that with the current mechanism.
> 
> But it's also not clear (to me at least), how is this better than what we had.
To make it clearer, here is the full error message for this instruction, with all of the context lines:

```
<stdin>:1:3: error: invalid instruction, multiple near-miss encodings found
  b #2
  ^
<stdin>:1:3: note: for one encoding: instruction requires: thumb
  b #2
  ^
<stdin>:1:5: note: for one encoding: invalid operand for instruction
  b #2
    ^
```

The two "note: for one encoding:" lines correspond to rows in the table-generated matcher table (which roughly corresponds to encodings in the ARMARM) which nearly match this instruction.

Two of these rows are Thumb branch instructions, for which an immediate of #2 is valid. I'm squashing these down into the one "instruction requires: thumb" message, as the second one wouldn't add anything.

Another of the rows is the ARM branch instruction, which doesn't match because the immediate must be a multiple of 4 (or a label, with alignment checked later). We don't currently have a diagnostic string for this operand class, so we emit the generic "invalid operand for instruction" message. Once this has been committed we can add a diagnostic string like "address needs to be 4-byte aligned" for the operand class.


================
Comment at: test/MC/ARM/diagnostics.s:183
         movwseq r9, #0xffff
-@ CHECK-ERRORS: error: immediate operand must be in the range [0,255]
+@ CHECK-ERRORS: error: invalid operand for instruction
 @ CHECK-ERRORS: error: instruction 'movw' can not set flags, but 's' suffix specified
----------------
rengolin wrote:
> Why did this one get worse?
This one was previously picking up the diagnostic for MCK_Imm0_255, which is only valid for Thumb targets (this test is using ARM mode). If the immediate was reduced, this would actually match the MCK_ModImm class. However, that doesn't have a diagnostic associated with it yet. Once it does, we'll get a more useful diagnostic than before.


================
Comment at: test/MC/ARM/fullfp16-neg.s:5
          vadd.f16  s0, s1, s0
-@ CHECK: error: instruction requires:
+@ CHECK: instruction requires:
 
----------------
rengolin wrote:
> requires what?
> 
> I know it was there already, but it's confusing. If we're trying to show off the new (better) error messages, we should display them in their full glory. :)
The full message is "instruction requires: full half-float", I'll update the test.

Many of these instructions also get an "invalid operand for instruction" error pointing to the ".f16" token, because they would be valid if they had ".f32" instead. I haven't updated the test to show this as I expect it to change once I add diagnostics for token operands.


================
Comment at: test/MC/ARM/invalid-fp-armv8.s:38
 vseleq.f32 s0, d2, d1
-@ V8: error: invalid operand for instruction
+@ V8: error: invalid instruction
 vselgt.f64 s3, s2, s1
----------------
rengolin wrote:
> This is technically "better" because you have chosen `-mattr=-neon`, but still doesn't say why the instruction is invalid.
> 
> Some error messages have the "requires ARMv8", but others don't. This one should know that it requires NEON, maybe a way to get the right info?
A lot of the instructions in this file (including this one) are invalid both because NEON is disabled, and because they have incorrect operands (so would not be correct even if NEON were enabled). We don't emit a more specific error here because neither change alone would make the instruction valid.


================
Comment at: test/MC/ARM/lsl-zero-errors.s:49
+// CHECK-NONARM: error: invalid instruction, multiple near-miss encodings found
+// CHECK-NONARM: invalid operand for instruction
 // CHECK-NONARM-NEXT: mov pc, r0, lsl #0
----------------
rengolin wrote:
> It says multiple near misses, but only shows one... Not very helpful.
A lot of the instructions in this test now get multiple near-misses (mostly different operands invalid, valid in other ISA), I'll update the test.


Repository:
  rL LLVM

https://reviews.llvm.org/D31530