[PATCH] D59733: ARM: Allow cp10/cp11 coprocessor register access with a warning

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added a comment.

I've made a couple of nitpicks to the patch, but my more general question is: if we think that //this// pair of coprocessors should be changed from "reject completely" to "accept with a warning", why not all of them? What argument applies to CP10 and CP11 that doesn't apply to all the others, apart from the immediate short-term argument of "this is the one that's currently causing somebody a problem"?



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.h:692
 
-  // Armv7 disallows 101x (CP10 and CP11), which clash with VFP/NEON.
-  if (featureBits[ARM::HasV7Ops] && (Num & 0xE) == 0xA)
-    return false;
-
   // Armv8.1-M also disallows 100x (CP8,CP9) and 111x (CP14,CP15)
   // which clash with MVE.
----------------
I think it's worth leaving a comment here to explain why we're //not// disallowing CP10 and CP11. The point of this function is to centralize all the coprocessor validity tests in one place, so I can easily imagine someone else coming along later and adding the "missing" one if there isn't something here telling them that it's omitted on purpose.


================
Comment at: llvm/test/MC/ARM/coprocessors.s:52
+@ ACCEPT-01234567CD: mrc   p10, #1, r2, c3, c4, #5
+@ REJECT-01234567CD: [[@LINE-2]]:7: error: invalid operand for instruction
 
----------------
This seems like a confusing way to change the test. The check prefix `ACCEPT-AB` is for test runs where coprocessors 10 and 11 (A and B) are accepted, and `REJECT-AB` for where they are rejected. So please leave these check lines as they were, so that each check prefix matches the message it's expecting, and instead change the set of `--check-prefix` options on the `llvm-mc` command whose behavior is changing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59733/new/

https://reviews.llvm.org/D59733