[ARM] Set Defs = [CPSR] for COPY_STRUCT_BYVAL, as it clobbers CPSR.

[fhahn]

Summary

Fixes PR35023.

[t.p.northover]

lib/CodeGen/ExpandISelPseudos.cpp

75

I think the logic needed here is a little more complicated. Fortunately, someone else has already been scarred for life by working it out (I expect) and you can just call addLiveIns from LivePhysRegs.h

[t.p.northover]

lib/CodeGen/ExpandISelPseudos.cpp

75

Actually, shouldn't whatever's doing the splitting update liveness too? For all ExpandISelPseudos knows there could be a dozen new blocks scattered across the function that need updating.

[qcolombet]

lib/CodeGen/ExpandISelPseudos.cpp

75

+1

[fhahn]

lib/CodeGen/ExpandISelPseudos.cpp

75

Actually, shouldn't whatever's doing the splitting update liveness too? For all ExpandISelPseudos knows there could be a dozen new blocks scattered across the function that need updating.

We could push the responsibility to the lowering code, but it would mean that we have to go through all lowering code. Doing it in ExpandISelLowering potentially misses cases as well though.

I'll take a look at the lowering code, a common pattern seems to be calling MBB->splice & MBB->transferSuccessorsAndPHIs in the lowering code (I checked ARM and X86). In some cases, there is manual handling for CPSR already, in some cases (like COPY_STRUCT_BYVAL_I32) it is missing.

[fhahn]

Use LivePhysRegs for liveins computation, pushed code to ARMISelLowering, handled all cases I could spot there. Adding Eli, as he fixed a similar problem in D54192, which I think should also be handled by the general liveins computation.

[efriedma]

I'm a little concerned computeAndAddLiveIns is more expensive than checkAndUpdateCPSRKill. In theory, they're both quadratic, but in practice I would expect checkAndUpdateCPSRKill is much cheaper: it only tracks one register, and quits immediately when it finds a def of CPSR.

[fhahn]

In D55909#1338002, @efriedma wrote:

I'm a little concerned computeAndAddLiveIns is more expensive than checkAndUpdateCPSRKill. In theory, they're both quadratic, but in practice I would expect checkAndUpdateCPSRKill is much cheaper: it only tracks one register, and quits immediately when it finds a def of CPSR.

Right, but don't we have to handle all relevant physical registers, e.g. if we would have a use of a physical register after tMOVCCr_pseudo in test_slt_not ? It might not be very likely in practice, but it seems like there is nothing preventing that from happening (it is quite easy to construct a MIR test that fails the machine verifier after expand-isel-pseudos). But maybe I am missing some constraint here?

The way we iterate over the instructions in ExpandISelPesudos also means that we end up doing more work than necessary if we have multiple pseudos introducing new MBBs in the original MBB, but we might be able to visit instructions in reverse order or something like that.

[efriedma]

The interesting constraint here is that ExpandISelPesudos runs immediately after ISel. And if you have an instruction which produces physical registers from an allocatable register class, ISel will COPY the values to virtual registers immediately after that instruction (and similarly for instructions that consume physical registers). So in practice, on ARM, the only physical register with a non-trivial live interval in ExpandISelPesudos is CPSR.

Granted, that's a bit fragile, and it isn't formally enforced anywhere.

[fhahn]

In D55909#1338092, @efriedma wrote:

The interesting constraint here is that ExpandISelPesudos runs immediately after ISel. And if you have an instruction which produces physical registers from an allocatable register class, ISel will COPY the values to virtual registers immediately after that instruction (and similarly for instructions that consume physical registers). So in practice, on ARM, the only physical register with a non-trivial live interval in ExpandISelPesudos is CPSR.

Granted, that's a bit fragile, and it isn't formally enforced anywhere.

Thanks Eli, I guess we can just go with only handling CPSR. I will update the patch!

[fhahn]

Reduced patch to just add CPSR to live-ins of newly created blocks, if required.

[efriedma]

lib/Target/ARM/ARMISelLowering.cpp

9009 ↗

(On Diff #179203)

Hang on... doesn't the loop clobber CPSR? I think rather than add the live-in marking like this, we need to mark COPY_STRUCT_BYVAL_I32 with "Defs = [CPSR]".

[qcolombet]

Could we just say that ExpandISelPseudo doesn't preserve the liveness and have it be recomputed when needed?

Basically, is the complexity we add here worth the compile time we save?
In particular, I am worried that we will do a lot of recomputations if there are a lot of copy_struct_by_val operations around, whereas we would do only one by recomputing the liveness lazily.

[fhahn]

lib/Target/ARM/ARMISelLowering.cpp

9009 ↗

(On Diff #179203)

Yes, at least when not in Thumb1, we define CPSR in the loop. I guess something could still potentially use CPSR after the COPY_STRUCT_BYVAL, then we would still need it in the live-ins. Not sure if that would make sense though.

[efriedma]

lib/Target/ARM/ARMISelLowering.cpp

9009 ↗

(On Diff #179203)

Presumably nothing is ever going to use the CPSR def of a COPY_STRUCT_BYVAL... we would have to add explicit logic for that, and the loop sets CPSR to a fixed value which isn't generally useful.

[fhahn]

Updated to just use Defs = [CPSR], as suggested by Eli.

[fhahn]

lib/Target/ARM/ARMISelLowering.cpp

9009 ↗

(On Diff #179203)

Right thanks, so for now I guess we can just mark it with Defs = [CPSR].

[efriedma]

LGTM

lib/CodeGen/ExpandISelPseudos.cpp

 //===-- llvm/CodeGen/ExpandISelPseudos.cpp ----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Expand Pseudo-instructions produced by ISel. These are usually to allow
 // the expansion to contain control flow, such as a conditional move
 // implemented with a conditional branch and a phi, or an atomic operation
 // implemented with a loop.
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetLowering.h"
+#include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
-#include "llvm/Support/Debug.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "expand-isel-pseudos"
 
 namespace {
   class ExpandISelPseudos : public MachineFunctionPass {
   public:
     static char ID; // Pass identification, replacement for typeid
for (MachineBasicBlock::iterator MBBI = MBB->begin(), MBBE = MBB->end();
         Changed = true;
         MachineBasicBlock *NewMBB = TLI->EmitInstrWithCustomInserter(MI, MBB);
         // The expansion may involve new basic blocks.
         if (NewMBB != MBB) {
           MBB = NewMBB;
           I = NewMBB->getIterator();
           MBBI = NewMBB->begin();
           MBBE = NewMBB->end();
+
+          // Calculate the LiveIns for the new MBB, by adding uses and removing
+          // defs while visiting the instructions in the new MBB in reverse
+          // order.
+          SmallSet<unsigned, 8> LiveIns;
+          for (MachineInstr &MI : reverse(*MBB)) {
+            for (MachineOperand &MOP : reverse(MI.operands())) {
+              if (!MOP.isReg() ||
+                  !TargetRegisterInfo::isPhysicalRegister(MOP.getReg()))
+                continue;
+              if (MOP.isDef())
+                LiveIns.erase(MOP.getReg());
+              else
+                LiveIns.insert(MOP.getReg());
+            }
+          }
+          for (unsigned R : LiveIns)
+            MBB->addLiveIn(R);
         }
       }
     }
   }
 
   return Changed;
 }

test/CodeGen/ARM/expand-isel-pseudos-liveins.mir

+# RUN: llc -run-pass=expand-isel-pseudos %s -o - | FileCheck %s
+--- |
+  target triple = "armv6kz"
+
+  define i32 @test1(i32 %x) {
+  entry:
+    unreachable
+  }
+
+...
+---
+name:            test1
+alignment:       2
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr, preferred-register: '' }
+  - { id: 1, class: gpr, preferred-register: '' }
+  - { id: 2, class: gpr, preferred-register: '' }
+  - { id: 3, class: gpr, preferred-register: '' }
+liveins:
+  - { reg: '$r0', virtual-reg: '%0' }
+  - { reg: '$r1', virtual-reg: '%1' }
+body:             |
+  bb.0.entry:
+    liveins: $r0, $r1
+    %0:gpr = COPY $r1
+    ADJCALLSTACKDOWN 136, 0, 14, $noreg, implicit-def dead $sp, implicit $sp
+    %1:gpr = COPY $sp
+    %2:gpr = COPY $r0
+    COPY_STRUCT_BYVAL_I32 %2, %0, 72, 8
+    %3:gpr = COPY $r1
+    STRi12 killed %3, %1, 132, 14, $noreg :: (store 4 into stack + 132)
+    ADJCALLSTACKUP 136, 0, 14, $noreg, implicit-def dead $sp, implicit $sp
+    BX_RET 14, $noreg
+...
+# CHECK-LABEL: name: test1
+# CHECK-LABEL: bb.2.entry:
+# CHECK-NEXT: liveins: $r1