[PATCH] D40362: [TableGen][AsmMatcherEmitter] Only choose specific diagnostic for enabled instruction

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

Thanks @renato for reminding of this patch. I'm a little concerned as some of the changes to the error messages are inconsistent. I'll take a longer look at this shortly but I've made some highlights below.



================
Comment at: test/MC/Mips/micromips/invalid-wrong-error.s:9-12
+  sdbbp -1            # CHECK: :[[@LINE]]:9: error: expected 10-bit unsigned immediate
   sdbbp 1024          # CHECK: :[[@LINE]]:3: error: instruction requires a CPU feature not currently enabled
-  syscall -1          # CHECK: :[[@LINE]]:11: error: expected 20-bit unsigned immediate
-  syscall $4          # CHECK: :[[@LINE]]:11: error: expected 20-bit unsigned immediate
+  syscall -1          # CHECK: :[[@LINE]]:11: error: expected 10-bit unsigned immediate
+  syscall $4          # CHECK: :[[@LINE]]:11: error: expected 10-bit unsigned immediate
----------------
These are improvements.


================
Comment at: test/MC/Mips/micromips32r6/invalid-wrong-error.s:10-42
+  teq $8, $9, $2           # CHECK: :[[@LINE]]:15: error: expected 4-bit unsigned immediate
+  teq $8, $9, -1           # CHECK: :[[@LINE]]:15: error: expected 4-bit unsigned immediate
   teq $8, $9, 16           # CHECK: :[[@LINE]]:3: error: instruction requires a CPU feature not currently enabled
-  tge $8, $9, $2           # CHECK: :[[@LINE]]:15: error: expected 10-bit unsigned immediate
-  tge $8, $9, -1           # CHECK: :[[@LINE]]:15: error: expected 10-bit unsigned immediate
+  tge $8, $9, $2           # CHECK: :[[@LINE]]:15: error: expected 4-bit unsigned immediate
+  tge $8, $9, -1           # CHECK: :[[@LINE]]:15: error: expected 4-bit unsigned immediate
   tge $8, $9, 16           # CHECK: :[[@LINE]]:3: error: instruction requires a CPU feature not currently enabled
+  tgeu $8, $9, $2          # CHECK: :[[@LINE]]:16: error: expected 4-bit unsigned immediate
----------------
These all look like improvements.


================
Comment at: test/MC/Mips/micromips64r6/invalid-wrong-error.s:18-51
+  teq $8, $9, $2           # CHECK: :[[@LINE]]:15: error: expected 4-bit unsigned immediate
+  teq $8, $9, -1           # CHECK: :[[@LINE]]:15: error: expected 4-bit unsigned immediate
   teq $8, $9, 16           # CHECK: :[[@LINE]]:3: error: instruction requires a CPU feature not currently enabled
-  tge $8, $9, $2           # CHECK: :[[@LINE]]:15: error: expected 10-bit unsigned immediate
-  tge $8, $9, -1           # CHECK: :[[@LINE]]:15: error: expected 10-bit unsigned immediate
+  tge $8, $9, $2           # CHECK: :[[@LINE]]:15: error: expected 4-bit unsigned immediate
+  tge $8, $9, -1           # CHECK: :[[@LINE]]:15: error: expected 4-bit unsigned immediate
   tge $8, $9, 16           # CHECK: :[[@LINE]]:3: error: instruction requires a CPU feature not currently enabled
+  tgeu $8, $9, $2          # CHECK: :[[@LINE]]:16: error: expected 4-bit unsigned immediate
----------------
These all look like improvements.


================
Comment at: test/MC/Mips/mips32r6/invalid-mips5-wrong-error.s:14
         ldc2 $1, 2048($12)            # CHECK: :[[@LINE]]:9: error: instruction requires a CPU feature not currently enabled
-        ldc2 $1, 1023($32)            # CHECK: :[[@LINE]]:18: error: expected memory with 16-bit signed offset
+        ldc2 $1, 1023($32)            # CHECK: :[[@LINE]]:18: error: expected memory with 11-bit signed offset
         lwc2 $1, -2049($4)            # CHECK: :[[@LINE]]:9: error: instruction requires a CPU feature not currently enabled
----------------
The error message here has improved. Unfortunately as we're rejecting the operand we aren't identifying which part of the operand is incorrect, but that's an issue for another day.


================
Comment at: test/MC/Mips/mips64/invalid-mips64r2.s:32-41
+        dins $2, $3, -1, 1            # CHECK: :[[@LINE]]:22: error: invalid operand for instruction
+        dins $2, $3, 64, 1            # CHECK: :[[@LINE]]:22: error: invalid operand for instruction
+        dinsm $2, $3, -1, 1           # CHECK: :[[@LINE]]:23: error: invalid operand for instruction
+        dinsm $2, $3, 32, 1           # CHECK: :[[@LINE]]:23: error: invalid operand for instruction
+        dinsm $2, $3, 31, 0           # CHECK: :[[@LINE]]:27: error: invalid operand for instruction
+        dinsm $2, $3, 31, 65          # CHECK: :[[@LINE]]:27: error: invalid operand for instruction
+        dinsu $2, $3, 31, 1           # CHECK: :[[@LINE]]:23: error: invalid operand for instruction
----------------
This is strictly a regression in terms of error quality. The ins* family of instructions have a number of non-intuitive constraints on the ranges of their immediate operands. "invalid operand for instruction" doesn't indicate what the operand should even be, whereas previously the exact permissible ranges where given.


================
Comment at: test/MC/Mips/msa/invalid.s:114-117
     copy_u.h $2, $w9[-1]     # CHECK: :[[@LINE]]:22: error: expected 3-bit unsigned immediate
     copy_u.h $2, $w9[8]      # CHECK: :[[@LINE]]:22: error: expected 3-bit unsigned immediate
-    copy_u.w $2, $w9[-1]     # CHECK: :[[@LINE]]:22: error: expected 2-bit unsigned immediate
-    copy_u.w $2, $w9[4]      # CHECK: :[[@LINE]]:22: error: expected 2-bit unsigned immediate
+    copy_u.w $2, $w9[-1]     # CHECK: :[[@LINE]]:22: error: invalid operand for instruction
+    copy_u.w $2, $w9[4]      # CHECK: :[[@LINE]]:22: error: invalid operand for instruction
----------------
This change isn't consistent. The copy_u.h's are just out of range of the number of vector elements and given a specific error. The copy_u.w are also out of range and given a generic error.


https://reviews.llvm.org/D40362