[PATCH] D29436: RegisterCoalescer: Fix joinReservedPhysReg()

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1825
+        // definition/valno of SrcReg.
+        for (SlotIndex SI = Indexes->getNextNonNullIndex(S.start);
+             SI < S.end; SI = Indexes->getNextNonNullIndex(SI)) {
----------------
I think starting from the next index is somewhat dangerous.  Consider this scenario: Hexagon has an instruction "J4_jumpsetr Rd, Rs, #bb", which is equivalent to "Rd = Rs; jump #bb".  This one will not cause a problem here, but it inspired me to invent a new hypothetical instruction where the branch is indirect: "J4_jumprsetr Rd, Rs, Rt", defined as "Rd = Rs; jumpr Rt".

I converted the testcase to Hexagon (use R30 instead of FP), with the change that it now uses the invented instruction to define virtual register:  "J4_jumprsetr %0, R0, R30". Before coalescing, this would be equivalent to "%0 = R0; jumpr R30", after coalescing it becomes "R30 = R0; jumpr R30".  While with the Hexagon semantics the R30 in the target of the branch would still be the old value, I'm not sure that this can be said about any potential architecture (which would support this kind of instructions).  Reserved physical registers may not obey the same use/def ordering, and there is no guarantee that the branch to R30 above would not be redirected to the newly defined value (or that is still a legal instruction to begin with).

Here's the modified testcase:

```
# RUN: llc -mtriple=hexagon -run-pass=simple-register-coalescing %s -o - | FileCheck %s
--- |
  define void @func1() { ret void }
...
---
name: func1
body: |
  bb.0:
    successors: %bb.3, %bb.4
    J2_jumpt undef %p0, %bb.3, implicit def %pc
    J2_jump %bb.4, implicit-def %pc

  bb.1:
    successors: %bb.2, %bb.5
    %r30 = COPY %0
    J2_jumpt undef %p0, %bb.2, implicit-def %pc
    J2_jump %bb.5, implicit-def %pc

  bb.2:
    successors: %bb.6
    %r30 = COPY %r0; outside the lifetime of %0, so shouldn't matter
    J2_jump %bb.6, implicit-def %pc

  bb.3:
    %r0 = COPY %r30 ; outside the lifetime of %0, so shouldn't matter
    J2_jumpr %r31, implicit-def %pc

  bb.5:
    S2_storeri_io %r30, 0, %r30
    J2_jumpr %r31, implicit-def %pc

  bb.6:
    J2_jumpr %r31, implicit-def %pc

  bb.4:
    successors: %bb.1
    %0 : intregs = J4_jumprsetr %r0, %r30, implicit-def %pc
...
```

You'd need to add a definition of it to the Hexagon target for the testcase to work, but you can change the testcase to use J4_jumpsetr (no "r" in the middle) to still get an instruction that uses R30.

Here's the output:

```
********** SIMPLE REGISTER COALESCING **********
********** Function: func1
********** JOINING INTERVALS ***********
(null):
64B     %R30<def> = COPY %vreg0; IntRegs:%vreg0
        Considering merging %vreg0 with %R30
                RHS = %vreg0 [48B,64r:0)[304r,320B:0)  0 at 304r
                Removing phys reg def of %R30 at 64B
                updated: 304B   %R30<def> = J4_jumprsetr %R0, %R30, %PC<imp-def>
        Success: %vreg0 -> %R30
        Result = %R30
(null):
(null):
128B    %R30<def> = COPY %R0
        Not coalescable.
(null):
(null):
176B    %R0<def> = COPY %R30
        Not coalescable.
(null):
(null):
Trying to inflate 0 regs.
```

and we end up with

```
288B    BB#6:
            Predecessors according to CFG: BB#0
304B            %R30<def> = J4_jumprsetr %R0, %R30, %PC<imp-def>
            Successors according to CFG: BB#1(0x80000000 / 0x80000000 = 100.00%)
```


Repository:
  rL LLVM

https://reviews.llvm.org/D29436