[ARM] Add an alias for psr and psr_nzcvq

[xiangzhai]

Summary

Hi LLVM developers,

As Eli suggested in the PR35213, I simply added an alias for psr register, please review my patch, thanks a lot!

Regards,
Leslie Zhai

[olista01]

Hi Leslie,

As far as I can tell, this is a gcc extension (it's not in any architecture reference manual I can find), which I can't find any documentation for. Is this correct, or is there some documentation I've missed? If I'm right, I'm not sure that there's any compelling reason to support this in LLVM. I've added Renato, who might know more about our policy for adding these sort of non-standard extensions.

If we do implement it, this isn't the right way to do it, you have given the "psr" operand a different encoding than "xpsr". In gcc's implementation, "psr" and "xpsr" result in exactly the same instructions. I've added Javed, who implemented this table-driver operand parsing, he might have some ideas about the best way to implement aliases.

As for the test, this is not the right file to put it in, I'd suggest adding it to test/MC/ARM/thumb2-mclass.s instead.

[javed.absar]

Hi Leslie:
As Oli mentioned, I did not find reference to PSR in ARM-ARM when preparing these tables. Leaving it to Renato to decide whether it should still be added.
If you do end up adding, and the encoding is same as XPSR, you could use the Mask bits 1,0,1 to differentiate against existing XPSR encoding entry (otherwise Searchable Table will complain about uniqueness).

[xiangzhai]

The same encoding as GNU Arm Embedded Toolchain:

$ arm-none-eabi-gcc -o thumb2-mclass.o -Wall -mlittle-endian -mcpu=cortex-m4 -march=armv7e-m -mthumb -mthumb-interwork -mfloat-abi=hard -mfpu=fpv4-sp-d16 -ffreestanding -c test/MC/ARM/thumb2-mclass.s

$ arm-none-eabi-objdump -d thumb2-mclass.o 

thumb2-mclass.o:     file format elf32-littlearm


Disassembly of section .text:

00000000 <.text>:
   0:   f3ef 8000       mrs     r0, CPSR
   4:   f3ef 8001       mrs     r0, IAPSR
   8:   f3ef 8002       mrs     r0, EAPSR
   c:   f3ef 8003       mrs     r0, PSR               <-- @ encoding: [0xef,0xf3,0x03,0x80]
  10:   f3ef 8005       mrs     r0, IPSR
  14:   f3ef 8006       mrs     r0, EPSR
  18:   f3ef 8007       mrs     r0, IEPSR
  1c:   f3ef 8008       mrs     r0, MSP
  20:   f3ef 8009       mrs     r0, PSP
  24:   f3ef 8010       mrs     r0, PRIMASK
  28:   f3ef 8014       mrs     r0, CONTROL
  2c:   f3ef 8c03       mrs     ip, PSR               <-- @ encoding: [0xef,0xf3,0x03,0x8c]
  30:   f380 8800       msr     CPSR_f, r0
  34:   f380 8800       msr     CPSR_f, r0
  38:   f380 8801       msr     IAPSR, r0
  3c:   f380 8801       msr     IAPSR, r0
  40:   f380 8802       msr     EAPSR, r0
  44:   f380 8802       msr     EAPSR, r0
  48:   f380 8803       msr     PSR, r0
  4c:   f380 8803       msr     PSR, r0
  50:   f380 8805       msr     IPSR, r0
  54:   f380 8806       msr     EPSR, r0
  58:   f380 8807       msr     IEPSR, r0
  5c:   f380 8808       msr     MSP, r0
  60:   f380 8809       msr     PSP, r0
  64:   f380 8810       msr     PRIMASK, r0
  68:   f380 8814       msr     CONTROL, r0

[xiangzhai]

Also add an alias for psr_nzcvq about this usecase.

[javed.absar]

lib/Target/ARM/ARMSystemRegister.td

67

Are there going to be more aliases required ? If so, we might be better of Adding a new mask for 'aliases'

[xiangzhai]

lib/Target/ARM/ARMSystemRegister.td

67

As GAS testsuite indicated, no more aliases required so far.

[kristof.beyls]

In D39712#917604, @olista01 wrote:

Hi Leslie,

As far as I can tell, this is a gcc extension (it's not in any architecture reference manual I can find), which I can't find any documentation for. Is this correct, or is there some documentation I've missed? If I'm right, I'm not sure that there's any compelling reason to support this in LLVM. I've added Renato, who might know more about our policy for adding these sort of non-standard extensions.

If we do implement it, this isn't the right way to do it, you have given the "psr" operand a different encoding than "xpsr". In gcc's implementation, "psr" and "xpsr" result in exactly the same instructions. I've added Javed, who implemented this table-driver operand parsing, he might have some ideas about the best way to implement aliases.

As for the test, this is not the right file to put it in, I'd suggest adding it to test/MC/ARM/thumb2-mclass.s instead.

FWIW, I don't think I've seen enough supporting material to add this to LLVM.
Is there any specification or documentation that documents "psr"?
What was the rationale for adding this seemingly un-specified extension to gas? Would the same rationale make us accept this patch in LLVM?
How much code in the world uses this? Couldn't just the pieces of code that use this get fixed? In that case, maybe providing a diagnostic indicating that "xpsr" should be used instead of "psr" is the most programmer-friendly option to implement in LLVM?
Just implementing more and more undocumented, non-standard and un-specified extensions to the assembler syntax will make the assembler worse, not better, IMHO.

[rengolin]

For reference, we tend not to add undocumented stuff. We only add GNU extensions when it's simple and necessary. We specifically don't add things that are implemented in GCC source code or test-cases, as that brings us bug-for-bug emulation, which we purposefully won't do.

This specific case smells like it's implemented in some old asm source (either asm file or inline) and people prefer to add "support" in the compiler rather than change the affected source code.

I'm inclined to agree with everyone else, this is not a good patch, regardless of how simple it is, it can open a can of worms.

So, unless compelling evidence can be shown that this functionality is mandatory, and won't add to the compiler's complexity in the long term, not just "easy to add", we should keep it out.

cheers,
--renato

[xiangzhai]

Provided a diagnostic indicating that "xpsr" should be used instead of "psr":

$ llvm/build/bin/llvm-mc -triple=thumbv7m test/MC/ARM/thumb2-mclass.s                
        .text




        mrs     r0, apsr
        mrs     r0, iapsr
        mrs     r0, eapsr
        mrs     r0, xpsr
        mrs     r0, ipsr
        mrs     r0, epsr
        mrs     r0, iepsr
        mrs     r0, msp
        mrs     r0, psp
        mrs     r0, primask
test/MC/ARM/thumb2-mclass.s:24:9: warning: xpsr should be used instead of psr
        mrs  ip, psr
        ^
        mrs     r0, control
        mrs     r12, xpsr



        msr     apsr_nzcvq, r0
        msr     apsr_nzcvq, r0
        msr     iapsr_nzcvq, r0
        msr     iapsr_nzcvq, r0
        msr     eapsr_nzcvq, r0
        msr     eapsr_nzcvq, r0
        msr     xpsr_nzcvq, r0
        msr     xpsr_nzcvq, r0
        msr     ipsr, r0
        msr     epsr, r0
        msr     iepsr, r0
        msr     msp, r0
        msr     psp, r0
        msr     primask, r0
test/MC/ARM/thumb2-mclass.s:58:9: warning: xpsr should be used instead of psr
        msr  psr_nzcvq, ip
        ^
        msr     control, r0
        msr     xpsr_nzcvq, r12

I am not familiar with the History https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=62b3e31101ef2dfb96ee4652d5145e722b335e31

[rengolin]

I'v looked everywhere and "psr" is not even pre-UAL. Please, fix the user code instead.

In D39712#922648, @xiangzhai wrote:

Provided a diagnostic indicating that "xpsr" should be used instead of "psr":

This is an arbitrary choice that I'm not ready to approve.

I am not familiar with the History https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=62b3e31101ef2dfb96ee4652d5145e722b335e31

This is a commit from more than 10 years ago in GCC. There is no evidence where the original author got the "psr" from and no documents supporting this choice.

As I said, we will most certainly not emulate GCC bug-for-bug.

The proper fix is to string-replace "psr" for "xpsr" in the user's code, which honestly, can't be that widely used, or we would have seen it by now.

cheers,
--renato

lib/Target/ARM/ARMSystemRegister.td

 def : MClassSysReg<0,    0,    1,    0x800, "apsr">;
 def : MClassSysReg<1,    1,    0,    0x800, "apsr_nzcvq">;
 def : MClassSysReg<0,    0,    1,    0x801, "iapsr">;
 def : MClassSysReg<1,    1,    0,    0x801, "iapsr_nzcvq">;
 def : MClassSysReg<0,    0,    1,    0x802, "eapsr">;
 def : MClassSysReg<1,    1,    0,    0x802, "eapsr_nzcvq">;
 def : MClassSysReg<0,    0,    1,    0x803, "xpsr">;
 def : MClassSysReg<1,    1,    0,    0x803, "xpsr_nzcvq">;
+def : MClassSysReg<0,    0,    1,    0x804, "psr">;
 
 def : MClassSysReg<0,    0,    1,    0x805, "ipsr">;
 def : MClassSysReg<0,    0,    1,    0x806, "epsr">;
 def : MClassSysReg<0,    0,    1,    0x807, "iepsr">;
 def : MClassSysReg<0,    0,    1,    0x808, "msp">;
 def : MClassSysReg<0,    0,    1,    0x809, "psp">;
 
 let Requires = [{ {ARM::HasV8MBaselineOps} }] in {
 def : MClassSysReg<0,    0,    1,    0x80a, "msplim">;

test/MC/ARM/arm-thumb-cpus-default.s

 @ RUN:  | FileCheck %s --check-prefix=CHECK-THUMB-ONLY
 
 @ RUN: llvm-mc -show-encoding -triple=armv6m-eabi < %s \
 @ RUN:  | FileCheck %s --check-prefix=CHECK-THUMB-ONLY
 
         @ Make sure the architecture chosen by LLVM defaults to a compatible
         @ ARM/Thumb mode.
         movs r0, r0
+        mrs ip, psr
 @ CHECK-ARM-THUMB: movs r0, r0 @ encoding: [0x00,0x00,0xb0,0xe1]
 @ CHECK-ARM-ONLY: movs r0, r0 @ encoding: [0x00,0x00,0xb0,0xe1]
 @ CHECK-THUMB-ONLY: movs r0, r0 @ encoding: [0x00,0x00]
+@ CHECK-THUMB-ONLY: mrs ip, psr @ encoding: [0xef,0xf3,0x04,0x8c]