[AArch64] Drop flags from BSP pseudos

[davemgreen]

This prevents cases where some of the operands match from hitting verifier errors with kill flags. These nodes should have been removed earlier in most cases.

Fixes the direct issue from #149380. #151855 cleans up the codegen.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

Author: David Green (davemgreen)

Changes

This prevents cases where some of the operands match from hitting verifier errorswith kill flags. These nodes should have been removed eariler in most cases.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp (+4-2)
• (modified) llvm/test/CodeGen/AArch64/bsp_implicit_ops.mir (+21-1)

diff --git a/llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp b/llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
index 201bfe0a443d6..77fcad634776d 100644
--- a/llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
@@ -1242,8 +1242,10 @@ bool AArch64ExpandPseudo::expandMI(MachineBasicBlock &MBB,
             .addReg(DstReg,
                     RegState::Define |
                         getRenamableRegState(MI.getOperand(0).isRenamable()))
-            .add(MI.getOperand(1))
-            .add(MI.getOperand(1));
+            .addReg(MI.getOperand(1).getReg(),
+                    getRenamableRegState(MI.getOperand(1).isRenamable()))
+            .addReg(MI.getOperand(1).getReg(),
+                    getRenamableRegState(MI.getOperand(1).isRenamable()));
         auto I2 =
             BuildMI(MBB, MBBI, MI.getDebugLoc(),
                     TII->get(Opcode == AArch64::BSPv8i8 ? AArch64::BSLv8i8
diff --git a/llvm/test/CodeGen/AArch64/bsp_implicit_ops.mir b/llvm/test/CodeGen/AArch64/bsp_implicit_ops.mir
index 23ac67cac6416..7ba363d46a1ff 100644
--- a/llvm/test/CodeGen/AArch64/bsp_implicit_ops.mir
+++ b/llvm/test/CodeGen/AArch64/bsp_implicit_ops.mir
@@ -13,7 +13,7 @@ body:             |
     ; CHECK-LABEL: name: BSL_COPY
     ; CHECK: liveins: $q20, $q21, $q22, $q23, $q6, $q1, $q7
     ; CHECK-NEXT: {{  $}}
-    ; CHECK-NEXT: renamable $q2 = ORRv16i8 killed renamable $q20, killed renamable $q20
+    ; CHECK-NEXT: renamable $q2 = ORRv16i8 renamable $q20, renamable $q20
     ; CHECK-NEXT: renamable $q2 = BSLv16i8 killed renamable $q2, renamable $q21, renamable $q6, implicit killed $q21_q22_q23, implicit killed $q0_q1_q2_q3, implicit-def $q0_q1_q2_q3
     ; CHECK-NEXT: $q22 = ORRv16i8 $q0, killed $q0
     ; CHECK-NEXT: $q23 = ORRv16i8 $q1, killed $q1
@@ -96,3 +96,23 @@ body:             |
     $q25 = ORRv16i8 $q3, killed $q3
     RET_ReallyLR implicit $q22
 ...
+---
+name:            DoubleOp
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    liveins: $q2
+
+    ; CHECK-LABEL: name: DoubleOp
+    ; CHECK: liveins: $q2
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: renamable $q0 = MOVIv8i16 1, 0
+    ; CHECK-NEXT: renamable $q1 = ORRv16i8 renamable $q2, renamable $q2
+    ; CHECK-NEXT: renamable $q1 = BSLv16i8 killed renamable $q1, renamable $q2, renamable $q0
+    ; CHECK-NEXT: renamable $q0 = SQADDv8i16 killed renamable $q1, killed renamable $q0
+    ; CHECK-NEXT: RET undef $lr, implicit $q0
+    renamable $q0 = MOVIv8i16 1, 0
+    renamable $q1 = BSPv16i8 killed renamable $q2, renamable $q2, renamable $q0
+    renamable $q0 = SQADDv8i16 killed renamable $q1, killed renamable $q0
+    RET_ReallyLR implicit $q0
+...

[rj-jesus]

Would it be worth preserving the kill flag when the MOV's operand isn't needed elsewhere, like in BSL_COPY above, and propagating it down to the BSL in other cases (for example, killing q2 here)?