[SystemZ] Drop regstate of SELRMux operand in selectSLRMux().

[JonPsson1]

It seems that there can be other cases with this that also can lead to wrong code (discovered with csmith). This time it involved not the kill flag but the undef flag.

It seems safest then to drop all the flags except for the renamable flag, which should always be true.

[llvmbot]

@llvm/pr-subscribers-backend-systemz

Author: Jonas Paulsson (JonPsson1)

Changes

It seems that there can be other cases with this that also can lead to wrong code (discovered with csmith). This time it involved not the kill flag but the undef flag.

It seems safest then to drop all the flags except for the renamable flag, which should always be true.

---

Full diff: https://github.com/llvm/llvm-project/pull/128555.diff

3 Files Affected:

• (modified) llvm/lib/Target/SystemZ/SystemZPostRewrite.cpp (+1-1)
• (modified) llvm/test/CodeGen/SystemZ/cond-move-10.mir (+1-1)
• (added) llvm/test/CodeGen/SystemZ/cond-move-11.mir (+43)

diff --git a/llvm/lib/Target/SystemZ/SystemZPostRewrite.cpp b/llvm/lib/Target/SystemZ/SystemZPostRewrite.cpp
index cf3073f0f2090..74c270f1c5018 100644
--- a/llvm/lib/Target/SystemZ/SystemZPostRewrite.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZPostRewrite.cpp
@@ -114,7 +114,7 @@ void SystemZPostRewrite::selectSELRMux(MachineBasicBlock &MBB,
   if (Src1Reg == Src2Reg) {
     BuildMI(*MBBI->getParent(), MBBI, MBBI->getDebugLoc(),
             TII->get(SystemZ::COPY), DestReg)
-        .addReg(MBBI->getOperand(1).getReg(), getRegState(MBBI->getOperand(1)));
+        .addReg(MBBI->getOperand(1).getReg(), RegState::Renamable);
     MBBI->eraseFromParent();
     return;
   }
diff --git a/llvm/test/CodeGen/SystemZ/cond-move-10.mir b/llvm/test/CodeGen/SystemZ/cond-move-10.mir
index 1db960829729e..7a27d8b02271f 100644
--- a/llvm/test/CodeGen/SystemZ/cond-move-10.mir
+++ b/llvm/test/CodeGen/SystemZ/cond-move-10.mir
@@ -5,7 +5,7 @@
 # CHECK:      name: fun0
 # CHECK:      renamable $r1l = AHIMuxK killed renamable $r1l, -1, implicit-def dead $cc
 # CHECK-NEXT: CHIMux renamable $r5h, 9, implicit-def $cc
-# CHECK-NEXT: $r14h = COPY killed renamable $r1l
+# CHECK-NEXT: $r14h = COPY renamable $r1l
 ---
 name:            fun0
 tracksRegLiveness: true
diff --git a/llvm/test/CodeGen/SystemZ/cond-move-11.mir b/llvm/test/CodeGen/SystemZ/cond-move-11.mir
new file mode 100644
index 0000000000000..a2431d9466fd1
--- /dev/null
+++ b/llvm/test/CodeGen/SystemZ/cond-move-11.mir
@@ -0,0 +1,43 @@
+# RUN: llc -o - %s -mtriple=s390x-linux-gnu -mcpu=z15 -start-before=systemz-post-rewrite \
+# RUN:   -stop-after=machine-cp 2>&1 | FileCheck %s
+
+# The chained SELRMux:es both has two operands with the same register but
+# where one of the operands have been marked as undef (resulting from
+# early-ifcvt). Check that the resulting COPY after machine-cp is from $r0l
+# to $r2l.
+
+# CHECK:      name: fun0
+# CHECK:      $r2l = COPY $r0l
+--- |
+  
+  @Res = global i32 0, align 4
+  @Z = global i32 0, align 4
+  define signext i32 @fun0() { ret i32 0 }
+...
+---
+name:            fun0
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.1(0x80000000)
+  
+    renamable $r0l = LRL @Z :: (dereferenceable load (s32) from @Z)
+    renamable $r1l = LHIMux 1
+  
+  bb.1:
+    successors: %bb.1(0x7c000000), %bb.2(0x04000000)
+    liveins: $r0l, $r1l
+  
+    CHIMux renamable $r1l, 0, implicit-def $cc
+    renamable $r2l = SELRMux undef renamable $r0l, renamable $r0l, 14, 6, implicit $cc
+    renamable $r2l = SELRMux undef renamable $r2l, killed renamable $r2l, 14, 6, implicit $cc
+    BRC 14, 8, %bb.1, implicit killed $cc
+    J %bb.2
+  
+  bb.2:
+    liveins: $r2l
+  
+    STRL renamable $r2l, @Res :: (store (s32) into @Res)
+    renamable $r2d = LGFR killed renamable $r2l
+    Return implicit $r2d
+...

[uweigand]

Hard-coding the renamable looks odd to me. It may be correct now, but who knows what's going on after possible future changes?

Since the same register is present as both source operands, but with differing regstate flags, can't we simply use the intersection of the flags from the two operands here?

[JonPsson1]

ok - makes sense. Also added the verifier to the test so that in any possible case where there might be a flag that requires one of a set of other flags this would hopefully be reported.

[uweigand]

LGTM, thanks!