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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Hi Oliver,

I'm still getting my bearings. The inline comments are more as a way to understand than to criticise your patch.

I think this is, in the long run, a good way forward. But I don't want to muddle the field before we get there.

So, before I look at the patch per se, we should discuss the error messages, what do they mean and what we can do.

Then later we can agree on multiple steps to get there, even if not all problems are fixed on the first commit (I'm sure they won't).

cheers,
--renato



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


================
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
----------------
Why did this one get worse?


================
Comment at: test/MC/ARM/fullfp16-neg.s:5
          vadd.f16  s0, s1, s0
-@ CHECK: error: instruction requires:
+@ CHECK: instruction requires:
 
----------------
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. :)


================
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
----------------
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?


================
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
----------------
It says multiple near misses, but only shows one... Not very helpful.


Repository:
  rL LLVM

https://reviews.llvm.org/D31530