access-softek / llvm-project

Other
0 stars 0 forks source link

[MSP430] Use the traditional R4 register name instead of FP pseudonym #12

Closed atrosinenko closed 4 years ago

atrosinenko commented 4 years ago

All references to FP inside comments seem to specifically refer it as a Frame Pointer, so no need to change them IMO.

I don't add a test as well, since it looks quite clumsy to have a test on register rename. On the other hand, it would be useful to have a test that checks all register names as used with llc but I don't know how to test it generically (while callee-saved registers obviously can be tested using inline asm).

asl commented 4 years ago

So, is the issue with clobber list is solved? So we could use r4 there?

atrosinenko commented 4 years ago

So, is the issue with clobber list is solved? So we could use r4 there?

Do you mean inability to write a test for that patch? If so, even before this PR, technically I could just clobber fp, r5, ..., r10 with the equivalent outcome. This test is not expected to change "what is possible, what is not".

On the other hand, we could want to rename fp to r4 for other reasons such as making it less surprising for other MSP430 backend developers (in a way, llc outputs r4 register name but accepts fp only). Then yes, it would be better to apply this before #8, otherwise that test would need an adjustment.

With this patch applied (without #8), the following change

--- a/r5-r10.ll
+++ b/r4-r10.ll
@@ -2,6 +2,6 @@ target datalayout = "e-m:e-p:16:16-i32:16-i64:16-f32:16-f64:16-a:8-n8:16-S16"
 target triple = "msp430"

 define i16 @pops_7_regs() nounwind {
-  call void asm sideeffect "", "~{r5},~{r6},~{r7},~{r8},~{r9},~{r10}"()
+  call void asm sideeffect "", "~{r4},~{r5},~{r6},~{r7},~{r8},~{r9},~{r10}"()
   ret i16 42
 }

causes the following asm listing change, as expected:

--- a/r5-r10.s
+++ b/r4-r10.s
@@ -11,9 +11,11 @@ pops_7_regs:                            ; @pops_7_regs
        push    r7
        push    r6
        push    r5
+       push    r4
        ;APP
        ;NO_APP
        mov     #42, r12
+       pop     r4
        pop     r5
        pop     r6
        pop     r7
asl commented 4 years ago

I actually wanted to ensure that: