[ARM] Correct POP reglist handling

[jyoti.allur]

Summary

ARM for v7-m states
POP
SP cannot be in the list

and GCC obliges with an error message for v7m
"Error: invalid register list to push/pop instruction -- `pop {sp}'"

[compnerd]

This is incorrect. pop {sp} is permitted on armv7-a and armv7-r. You are correct, I did not correctly account for armv7-m. This would result in incorrect behavior on the A/R targets.

[jyoti.allur]

Hi Compnerd,

Some tests with GNU assembler lead me to think pop {sp} is not permitted in v7 a/r targets as well in thumb mode though.

GNU assembler results :-

cat ldmstm.s
pop {r4, sp}

POP(Thumb) mode v7 a/r

arm-linux-gnueabi-as -march=armv7r -mthumb ldmstm.s
ldmstm.s: Assembler messages:
ldmstm.s:12: Error: invalid register list to push/pop instruction -- `pop {r4,sp}'

arm-linux-gnueabi-as -march=armv7a -mthumb ldmstm.s
ldmstm.s: Assembler messages:
ldmstm.s:12: Error: invalid register list to push/pop instruction -- `pop {r4,sp}'

Without this patch, SP was allowed in POP(Thumb mode) for v7 A/R targets, resulting in
pop.w {r4, sp}

This patch does not affect POP(ARM mode) for v7 A/R targets as it is handled in getARMLoadDeprecationInfo.
Let me know if the correction should still be done for v7m alone? i'll modify accordingly.

[compnerd]

The current revision of the ARM ARM seems to indicate that pop {sp} is permitted. I think that this may be a bug in GAS (its tricky since ldm {sp} is not permitted, but pop can be relaxed to a ldm). I think for now, being more conservative and just changing the IsPop to IsARPop and passing that information along might be the better solution.

[jyoti.allur]

Alright, modified accordingly.
sub arch and arch profile information is unavailable in AsmParser, i have made use of the feature bits for MClass to know whether POP is for AR targets or not. Consequently, I had no choice but to to replicate the testcases for v7m.
A and R are defined by same manual, so i assume no need to test for R seperately.
Thanks.

[jyoti.allur]

Hi Saleem,
Could you please review this, shouldn't take much of you time as changes are minimal.
Thanks!

[jroelofs]

../llvm/test/MC/ARM/thumb-load-store-multiple.s

4

Instead of duplicating all of the shared cases below, how about:

@ RUN: not llvm-mc -triple thumbv7a-eabi -filetype asm -o - %s 2>&1 \
@ RUN: | FileCheck --check-prefix=CHECK --check-prefix=CHECK-V7A %s
@ RUN: not llvm-mc -triple thumbv7m-eabi -filetype asm -o - %s 2>&1 \
@ RUN: | FileCheck --check-prefix=CHECK --check-prefix=CHECK-V7M %s

Then only split out the cases that are different. I think that will significantly reduce the diff in this file.

[compnerd]

I agree with jroelofs' suggestion about the test changes. I think that would make it easier to maintain as well.

../llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6330

The variable here is unnecessary. I would just inline the call to !isMClass() not the call to validateLDMRegList.

[jyoti.allur]

Ah. why din't i think about it. Thanks Jonathan and Saleem.
Updated the diff.

[compnerd]

Thanks for addressing all the review comments.

../llvm/test/MC/ARM/thumb-load-store-multiple.s

13

oops.

34

Ugh. Thanks for fixing this.

[jyoti.allur]

Thanks for reviewing.
Closed by commit rL225972 (authored @jyoti.allur )

../llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

   }
 
   bool validatetLDMRegList(MCInst Inst, const OperandVector &Operands,
-                           unsigned ListNo, bool IsPop = false);
+                           unsigned ListNo);
   bool validatetSTMRegList(MCInst Inst, const OperandVector &Operands,
                            unsigned ListNo);
 

 
 bool ARMAsmParser::validatetLDMRegList(MCInst Inst,
                                        const OperandVector &Operands,
-                                       unsigned ListNo, bool IsPop) {
+                                       unsigned ListNo) {
   const ARMOperand &Op = static_cast<const ARMOperand &>(*Operands[ListNo]);
   bool HasWritebackToken = Op.isToken() && Op.getToken() == "!";
 

   bool ListContainsLR = listContainsReg(Inst, ListNo, ARM::LR);
   bool ListContainsPC = listContainsReg(Inst, ListNo, ARM::PC);
 
-  if (!IsPop && ListContainsSP)
+  if (ListContainsSP)
     return Error(Operands[ListNo + HasWritebackToken]->getStartLoc(),
                  "SP may not be in the register list");
   else if (ListContainsPC && ListContainsLR)

         !isThumbTwo())
       return Error(Operands[2]->getStartLoc(),
                    "registers must be in range r0-r7 or pc");
-    if (validatetLDMRegList(Inst, Operands, 2, /*IsPop=*/true))
+    if (validatetLDMRegList(Inst, Operands, 2))
       return true;
     break;
   }

../llvm/test/MC/ARM/thumb-load-store-multiple.s

 
   .global ldm
   .type ldm,%function
-ldb:
+ldm:
   ldm r0!, {r1, sp}
 @ CHECK: error: SP may not be in the register list
 @ CHECK: ldm r0!, {r1, sp}

 ldmdb:
   ldmdb r0!, {r1, sp}
 @ CHECK: error: SP may not be in the register list
-  ldm r0!, {lr, pc}
+  ldmdb r0!, {lr, pc}
 @ error: PC and LR may not be in the register list simultaneously
   itt eq
   ldmeq r0!, {r1, pc}

 @ CHECK: error: SP may not be in the register list
   push {pc}
 @ CHECK: error: PC may not be in the register list
-  push {sp,pc}
+  push {sp, pc}
 @ CHECK: error: SP and PC may not be in the register list
 
   .global pop
   .type pop,%function
 pop:
+        pop {sp}
+@ CHECK: error: SP may not be in the register list
   pop {lr, pc}
 @ CHECK: error: PC and LR may not be in the register list simultaneously
 @ CHECK: pop {lr, pc}

   .global valid
   .type valid,%function
 valid:
-  pop {sp}
-@ CHECK: ldr sp, [sp], #4
-  pop {sp, pc}
-@ CHECK: pop.w {sp, pc}
   push.w {r0}
 @ CHECK: str r0, [sp, #-4]
   pop.w {r0}