Fix for PR20104. Enable GAS compliant coprocessor register name.

[akuharev]

Summary

Added parsing for parameter names starting with "cr"

Additional compliant GAS names for coprocessor register name
are enabled for all instruction with parameter MCK_CoprocReg:
LDC,LDC2,STC,STC2,CDP,CDP2,MCR,MCR2,MCRR,MCRR2,MRC,MRC2,MRRC,MRRC2
...

[dyatkovskiy]

Hi Andrey,
Thanks for working on it!
I think we have to keep compatibility with GAS.. So in general patch looks reasonable. May be it would be good to add some details what method does (see inline comments).
I've CCed Renato and Tim for approval.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

3085–3092

I think, it would be good to explain the grammar in short, something like:
RegName ::= <Prefix><Number>
Number - integer in range [0, 16)
If CoprocOp is 'c', then:
Prefix ::= c|cr
If CoprocOp is 'c', then:
Prefix ::= p

3099

Please place comment above "if" statement:
// For gas compatibility
if ...

[rengolin]

Hi Andrey,

Addressing both mine and Stepan's comments, the patch looks good to me.

cheers,
--renato

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

3099

Since we use CoprocOp for the first char, I'd do in a similar way for the GNU compatibility.

Alternatively, assuming the name will always come lower case, and since you already tested for the first char, I'd just test:

if (Name[1] == 'r') {
  Name = Name.drop_front();
  Name[0] = CoprocOp;
}

Why the second line? Because from this point onwards, the string will be 'rN' rather than 'cN', which is why you moved the CoprocOp test upwards, but that is not future proof, and anyone writing checks below that line will get confused, or produce confusing code, such as:

if (Name[0] == 'r') ...

which doesn't help future developers.

[akuharev]

Added comments and cut prefix in switch

[akuharev]

Hi Renato,

StringRef is wrapper under constant string and it's not acceptable:
Name[0] = CoprocOp;

It may be better to cut off prefix and parse only numbers in switch.

-Andrey

[rengolin]

Indeed, much better! LGTM.

Thanks,
--renato

[akuharev]

Thanks for reviews!
I don't have commit access. Please commit the patch.

[dyatkovskiy]

Yup, Renato proposed good idea. Final patch looks fine, except tiny thing, could you use single drop_front call (see inline comments)?

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

3101

May be like this?
Name = (Name[1] == 'r') ? Name.drop_front(2) : Name.drop_front();

[akuharev]

Reduced

[rengolin]

LGTM,

Thanks!
--renato

[dyatkovskiy]

Andrey,
do you need us to commit it?

[akuharev]

Thanks for the review!
Could you commit the patch?
I don't have commit access.

-Andrey

[rengolin]

Committed in r211776.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

Operands.push_back(ARMOperand::CreateVectorIndex(MCE->getValue(),
                                                      SIdx, E,
                                                      getContext()));
   }
 
   return false;
 }
 
 /// MatchCoprocessorOperandName - Try to parse an coprocessor related
-/// instruction with a symbolic operand name. Example: "p1", "p7", "c3",
-/// "c5", ...
+/// instruction with a symbolic operand name. 
+/// We accept "crN" syntax for GAS compatibility.
+/// <operand-name> ::= <prefix><number> 
+/// If CoprocOp is 'c', then:
+///   <prefix> ::= c | cr
+/// If CoprocOp is 'p', then :
+///   <prefix> ::= p
+/// <number> ::= integer in range [0, 15]
 static int MatchCoprocessorOperandName(StringRef Name, char CoprocOp) {
   // Use the same layout as the tablegen'erated register name matcher. Ugly,
   // but efficient.
+  if (Name.size() < 2 || Name[0] != CoprocOp)
+    return -1;
+  Name = (Name[1] == 'r') ? Name.drop_front(2) : Name.drop_front();
+
   switch (Name.size()) {
   default: return -1;
-  case 2:
-    if (Name[0] != CoprocOp)
-      return -1;
-    switch (Name[1]) {
+  case 1:
+    switch (Name[0]) {
     default:  return -1;
     case '0': return 0;
     case '1': return 1;
     case '2': return 2;
     case '3': return 3;
     case '4': return 4;
     case '5': return 5;
     case '6': return 6;
     case '7': return 7;
     case '8': return 8;
     case '9': return 9;
     }
-  case 3:
-    if (Name[0] != CoprocOp || Name[1] != '1')
+  case 2:
+    if (Name[0] != '1')
       return -1;
-    switch (Name[2]) {
+    switch (Name[1]) {
     default:  return -1;
     // p10 and p11 are invalid for coproc instructions (reserved for FP/NEON)
     case '0': return CoprocOp == 'p'? -1: 10;
     case '1': return CoprocOp == 'p'? -1: 11;
     case '2': return 12;
     case '3': return 13;
     case '4': return 14;
     case '5': return 15;

test/MC/ARM/gas-compl-copr-reg.s

+@ RUN: llvm-mc -triple=armv7-linux-gnueabi -show-encoding < %s | FileCheck %s
+
+@ CHECK: ldc  p12, c4, [r0, #4]       @ encoding: [0x01,0x4c,0x90,0xed]
+@ CHECK: stc  p14, c6, [r2, #-224]    @ encoding: [0x38,0x6e,0x02,0xed]
+
+        ldc p12, cr4, [r0, #4]
+        stc p14, cr6, [r2, #-224]