[branchfolding] When hoisting common code, remove kill flags from uses

[Ka-Ka]

Summary

The branch folder wrongly leave kill flags that the verifier
complain about. This can only occur on targets with super
registers where you can write the parts individually. The reason
for this is that the function addRegAndItsAliases() that build
the ActiveDefsSet will consider the super register defed if one
of the part are defed. The easy way to solve this is to be
conservative and always remove kill flags from uses if the killed
registers are needed below the insertion point, regardless of if
registers exist in the ActiveDefsSet or not.

This is a conservative refinement of the old fix in r148043

[jonpa]

The test case seems to work fine when I tried it w and w/out the patch.

Adding Uli as a reviewer for the test case, since it's SystemZ (he always reviews SystemZ changes). Note: as Kalle explained to me in a mail, this test case has been somewhat modified to expose the bug on SystemZ, but since this is a .mir test case which passes the machine verifier, I think this is not a problem.

Some reviewer for BranchFolding.cpp still needed.

[uweigand]

The test case looks fine to me, thanks.

[Ka-Ka]

In D46048#1079135, @jonpa wrote:

Adding Uli as a reviewer for the test case, since it's SystemZ (he always reviews SystemZ changes). Note: as Kalle explained to me in a mail, this test case has been somewhat modified to expose the bug on SystemZ, but since this is a .mir test case which passes the machine verifier, I think this is not a problem.

The fault was originally found in a out of tree target, but I was able to reproduce the fault on SystemZ as my out of tree target and SystemZ share the same register characteristics (the register r0d is a pair of r0h and r0l and they can be written to separately) needed to trigger the fault.

[Ka-Ka]

Adding Matthias Braun and Krzysztof Parzyszek as reviewer as it seems you both have done work in BranchFolding.cpp before. Do you have time to review this change?

[Ka-Ka]

Ping!
Anyone up for reviewing this?

[Ka-Ka]

Ping?

lib/CodeGen/BranchFolding.cpp

for (MachineOperand &MO : TIB->operands()) {
           break;
         }
       } else if (!ActiveDefsSet.count(Reg)) {
         if (Defs.count(Reg)) {
           // Use is defined by the instruction at the point of insertion.
           IsSafe = false;
           break;
         }
-
-        if (MO.isKill() && Uses.count(Reg))
-          // Kills a register that's read by the instruction at the point of
-          // insertion. Remove the kill marker.
-          MO.setIsKill(false);
       }
     }
     if (!IsSafe)
       break;
 
     bool DontMoveAcrossStore = true;
     if (!TIB->isSafeToMove(nullptr, DontMoveAcrossStore))
       break;
 
     // Remove kills from ActiveDefsSet, these registers had short live ranges.
-    for (const MachineOperand &MO : TIB->operands()) {
+    for (MachineOperand &MO : TIB->operands()) {
       if (!MO.isReg() || !MO.isUse() || !MO.isKill())
         continue;
       unsigned Reg = MO.getReg();
       if (!Reg)
         continue;
+      if (Uses.count(Reg))
+        // Kills a register that's read by the instruction at the point of
+        // insertion. Remove the kill marker.
+        MO.setIsKill(false);
       if (!AllDefsSet.count(Reg)) {
         LocalKills.push_back(Reg);
         continue;
       }
       if (TargetRegisterInfo::isPhysicalRegister(Reg)) {
         for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
           ActiveDefsSet.erase(*AI);
       } else {

test/CodeGen/SystemZ/branch-folder-hoist.mir

+# RUN: llc -verify-machineinstrs -mtriple=s390x-ibm-linux -o - %s -run-pass=branch-folder | FileCheck %s
+
+--- |
+  target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64"
+  target triple = "s390x-ibm-linux"
+
+  @d = external dso_local global i64, align 1
+  @o = external dso_local local_unnamed_addr global i16, align 1
+  @i = external dso_local local_unnamed_addr global i16, align 1
+  @g = external dso_local local_unnamed_addr global i32, align 1
+
+  define dso_local void @f3() local_unnamed_addr {
+  entry:
+    ret void
+  }
+
+...
+---
+name:            f3
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.2(0x30000000), %bb.3(0x50000000)
+    liveins: $r1d, $r2l
+
+    renamable $r3d = LGRL @d :: (load 4 from got, align 8)
+    renamable $r0d = LGF renamable $r1d, 0, $noreg :: (dereferenceable load 2 from @g, align 1)
+    dead renamable $r3d = LG killed renamable $r3d, 0, $noreg :: (volatile dereferenceable load 4 from @d, align 1)
+    TMLL renamable $r0l, 65535, implicit-def $cc
+    BRC 15, 7, %bb.3, implicit killed $cc
+    J %bb.2
+
+  bb.2:
+    successors: %bb.4(0x80000000)
+    liveins: $r0d, $r1d, $r2l
+
+    $r0h = COPY $r0l
+    $r3d = COPY killed $r0d
+    renamable $r3d = LGRL @o :: (load 4 from got, align 8)
+    MVHHI killed renamable $r3d, 0, 0 :: (store 1 into @o)
+    J %bb.4
+
+  bb.3:
+    successors: %bb.4(0x80000000)
+    liveins: $r0d, $r1d, $r2l
+
+    $r0h = COPY $r0l
+    $r3d = COPY killed $r0d
+    renamable $r3d = LGRL @i :: (load 4 from got, align 8)
+    MVHHI killed renamable $r3d, 0, 0 :: (store 1 into @i)
+
+  bb.4:
+    liveins: $r1d, $r2l
+
+    CHI killed renamable $r2l, 0, implicit-def $cc
+    renamable $r2d = LGHI 5
+    Return
+...
+
+# Note: the second COPY should not have killed flag on $r0d as $r0l is used in
+# the TMLL instruction
+
+# CHECK: bb.0:
+# CHECK: $r0h = COPY $r0l
+# CHECK-NEXT: $r3d = COPY $r0d
+# CHECK-NEXT: TMLL renamable $r0l, 65535, implicit-def $cc