release/22.x: MIPS/expandAtomicBinOp: Remove tailing kill dead register operands (#186055)

[llvmbot]

Backport d2c6e4c

Requested by: @wzssyqa

[llvmbot]

@llvm/pr-subscribers-backend-mips

Author: None (llvmbot)

Changes

Backport d2c6e4c

Requested by: @wzssyqa

---

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

2 Files Affected:

• (modified) llvm/lib/Target/Mips/MipsExpandPseudo.cpp (+27)
• (added) llvm/test/CodeGen/Mips/atomic-min-max-LiveVariables.ll (+131)

diff --git a/llvm/lib/Target/Mips/MipsExpandPseudo.cpp b/llvm/lib/Target/Mips/MipsExpandPseudo.cpp
index 78f2e5db40f9d..b39eb9863b042 100644
--- a/llvm/lib/Target/Mips/MipsExpandPseudo.cpp
+++ b/llvm/lib/Target/Mips/MipsExpandPseudo.cpp
@@ -882,6 +882,33 @@ bool MipsExpandPseudo::expandAtomicBinOp(MachineBasicBlock &BB,
   assert((OldVal != Ptr) && "Clobbered the wrong ptr reg!");
   assert((OldVal != Incr) && "Clobbered the wrong reg!");
   if (IsMin || IsMax) {
+    // Remove trailing kill/dead register operands added by
+    // MachineInstr::addRegisterKilled. These are super-register markers that
+    // must correspond to one of the physical registers in operands 1-4.
+    // The kill/dead markers may also appear on preceding subregs.
+    const TargetRegisterInfo *TRI = STI->getRegisterInfo();
+    while (I->getNumOperands() > 5) {
+      auto &Op = I->getOperand(I->getNumOperands() - 1);
+      // Check if this register overlaps with any physical register in
+      // operands 1-4 that has kill/dead marker (i.e., it's a super-register
+      // marker for subregs).
+      bool HasOverlapWithKill = false;
+      for (unsigned i = 1; i <= 4; ++i) {
+        auto &RefOp = I->getOperand(i);
+        if (RefOp.isReg() && RefOp.getReg().isPhysical() &&
+            TRI->regsOverlap(Op.getReg(), RefOp.getReg()) &&
+            (RefOp.isKill() || RefOp.isDead())) {
+          HasOverlapWithKill = true;
+          break;
+        }
+      }
+      // Remove if HasOverlapWithKill is true or Op has kill/dead marker.
+      bool HasKillOrDead = Op.isReg() && Op.getReg().isPhysical() &&
+                           (Op.isKill() || Op.isDead());
+      if (!HasOverlapWithKill && !HasKillOrDead)
+        break;
+      I->removeOperand(I->getNumOperands() - 1);
+    }
 
     assert(I->getNumOperands() == 5 &&
            "Atomics min|max|umin|umax use an additional register");
diff --git a/llvm/test/CodeGen/Mips/atomic-min-max-LiveVariables.ll b/llvm/test/CodeGen/Mips/atomic-min-max-LiveVariables.ll
new file mode 100644
index 0000000000000..d07c7f61ece41
--- /dev/null
+++ b/llvm/test/CodeGen/Mips/atomic-min-max-LiveVariables.ll
@@ -0,0 +1,131 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=mips64-elf -O2 -mcpu=mips64r2 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=R2-O2
+; RUN: llc -mtriple=mips64-elf -O3 -mcpu=mips64r2 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=R2-O3
+; RUN: llc -mtriple=mips64el-elf -O2 -mcpu=mips64r2 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=R2-O2
+; RUN: llc -mtriple=mips64el-elf -O3 -mcpu=mips64r2 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=R2-O3
+; RUN: llc -mtriple=mips64-elf -O2 -mcpu=mips64r6 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=R6-O2
+; RUN: llc -mtriple=mips64-elf -O3 -mcpu=mips64r6 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=R6-O3
+; RUN: llc -mtriple=mips64el-elf -O2 -mcpu=mips64r6 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=R6-O2
+; RUN: llc -mtriple=mips64el-elf -O3 -mcpu=mips64r6 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=R6-O3
+
+declare void @exit(i32) noreturn
+
+define void @test_func(ptr %ptr, i32 %val, i1 %cond) nounwind {
+; R2-O2-LABEL: test_func:
+; R2-O2:       # %bb.0: # %entry
+; R2-O2-NEXT:    move $2, $4
+; R2-O2-NEXT:    sll $1, $6, 0
+; R2-O2-NEXT:    andi $1, $1, 1
+; R2-O2-NEXT:    beqz $1, .LBB0_4
+; R2-O2-NEXT:    sll $4, $5, 0
+; R2-O2-NEXT:  # %bb.1: # %bb5
+; R2-O2-NEXT:    .insn
+; R2-O2-NEXT:  .LBB0_2: # %bb5
+; R2-O2-NEXT:    # =>This Inner Loop Header: Depth=1
+; R2-O2-NEXT:    ll $1, 0($2)
+; R2-O2-NEXT:    sltu $5, $1, $4
+; R2-O2-NEXT:    move $3, $1
+; R2-O2-NEXT:    movn $3, $4, $5
+; R2-O2-NEXT:    sc $3, 0($2)
+; R2-O2-NEXT:    beqz $3, .LBB0_2
+; R2-O2-NEXT:    nop
+; R2-O2-NEXT:  # %bb.3: # %bb5
+; R2-O2-NEXT:    jr $ra
+; R2-O2-NEXT:    nop
+; R2-O2-NEXT:  .LBB0_4: # %bb4
+; R2-O2-NEXT:    daddiu $sp, $sp, -16
+; R2-O2-NEXT:    sd $ra, 8($sp) # 8-byte Folded Spill
+; R2-O2-NEXT:    jal exit
+; R2-O2-NEXT:    nop
+;
+; R2-O3-LABEL: test_func:
+; R2-O3:       # %bb.0: # %entry
+; R2-O3-NEXT:    sll $1, $6, 0
+; R2-O3-NEXT:    move $2, $4
+; R2-O3-NEXT:    andi $1, $1, 1
+; R2-O3-NEXT:    beqz $1, .LBB0_4
+; R2-O3-NEXT:    sll $4, $5, 0
+; R2-O3-NEXT:  # %bb.1: # %bb5
+; R2-O3-NEXT:    .insn
+; R2-O3-NEXT:  .LBB0_2: # %bb5
+; R2-O3-NEXT:    # =>This Inner Loop Header: Depth=1
+; R2-O3-NEXT:    ll $1, 0($2)
+; R2-O3-NEXT:    sltu $5, $1, $4
+; R2-O3-NEXT:    move $3, $1
+; R2-O3-NEXT:    movn $3, $4, $5
+; R2-O3-NEXT:    sc $3, 0($2)
+; R2-O3-NEXT:    beqz $3, .LBB0_2
+; R2-O3-NEXT:    nop
+; R2-O3-NEXT:  # %bb.3: # %bb5
+; R2-O3-NEXT:    jr $ra
+; R2-O3-NEXT:    nop
+; R2-O3-NEXT:  .LBB0_4: # %bb4
+; R2-O3-NEXT:    daddiu $sp, $sp, -16
+; R2-O3-NEXT:    sd $ra, 8($sp) # 8-byte Folded Spill
+; R2-O3-NEXT:    jal exit
+; R2-O3-NEXT:    nop
+;
+; R6-O2-LABEL: test_func:
+; R6-O2:       # %bb.0: # %entry
+; R6-O2-NEXT:    move $2, $4
+; R6-O2-NEXT:    sll $1, $6, 0
+; R6-O2-NEXT:    andi $1, $1, 1
+; R6-O2-NEXT:    beqz $1, .LBB0_4
+; R6-O2-NEXT:    sll $4, $5, 0
+; R6-O2-NEXT:  # %bb.1: # %bb5
+; R6-O2-NEXT:    .insn
+; R6-O2-NEXT:  .LBB0_2: # %bb5
+; R6-O2-NEXT:    # =>This Inner Loop Header: Depth=1
+; R6-O2-NEXT:    ll $1, 0($2)
+; R6-O2-NEXT:    sltu $5, $1, $4
+; R6-O2-NEXT:    seleqz $3, $1, $5
+; R6-O2-NEXT:    selnez $5, $4, $5
+; R6-O2-NEXT:    or $3, $3, $5
+; R6-O2-NEXT:    sc $3, 0($2)
+; R6-O2-NEXT:    beqzc $3, .LBB0_2
+; R6-O2-NEXT:    nop
+; R6-O2-NEXT:  # %bb.3: # %bb5
+; R6-O2-NEXT:    jrc $ra
+; R6-O2-NEXT:  .LBB0_4: # %bb4
+; R6-O2-NEXT:    daddiu $sp, $sp, -16
+; R6-O2-NEXT:    sd $ra, 8($sp) # 8-byte Folded Spill
+; R6-O2-NEXT:    jal exit
+; R6-O2-NEXT:    nop
+;
+; R6-O3-LABEL: test_func:
+; R6-O3:       # %bb.0: # %entry
+; R6-O3-NEXT:    sll $1, $6, 0
+; R6-O3-NEXT:    move $2, $4
+; R6-O3-NEXT:    andi $1, $1, 1
+; R6-O3-NEXT:    beqz $1, .LBB0_4
+; R6-O3-NEXT:    sll $4, $5, 0
+; R6-O3-NEXT:  # %bb.1: # %bb5
+; R6-O3-NEXT:    .insn
+; R6-O3-NEXT:  .LBB0_2: # %bb5
+; R6-O3-NEXT:    # =>This Inner Loop Header: Depth=1
+; R6-O3-NEXT:    ll $1, 0($2)
+; R6-O3-NEXT:    sltu $5, $1, $4
+; R6-O3-NEXT:    seleqz $3, $1, $5
+; R6-O3-NEXT:    selnez $5, $4, $5
+; R6-O3-NEXT:    or $3, $3, $5
+; R6-O3-NEXT:    sc $3, 0($2)
+; R6-O3-NEXT:    beqzc $3, .LBB0_2
+; R6-O3-NEXT:    nop
+; R6-O3-NEXT:  # %bb.3: # %bb5
+; R6-O3-NEXT:    jrc $ra
+; R6-O3-NEXT:  .LBB0_4: # %bb4
+; R6-O3-NEXT:    daddiu $sp, $sp, -16
+; R6-O3-NEXT:    sd $ra, 8($sp) # 8-byte Folded Spill
+; R6-O3-NEXT:    jal exit
+; R6-O3-NEXT:    nop
+entry:
+  br i1 %cond, label %bb5, label %bb4
+
+bb4:
+  call void @exit(i32 %val)
+  unreachable
+
+bb5:
+  %old = atomicrmw umax ptr %ptr, i32 %val monotonic, align 4
+  ret void
+}

[dyung]

@wzssyqa can you provide some information on this change? I notice that you were the author of the original patch and it was not reviewed and I'm not sure who would be best to review this.

Is this a regression? If so, when was the regression introduced. What are the consequences of not accepting this patch in the release branch? Will it cause bad code gen?

[alexrp]

This is blocking us from enabling ABI-conforming sign extension in Zig: https://codeberg.org/ziglang/zig/src/commit/54b3484256695381c034aa30f322b1499a4b27c8/src/codegen/llvm.zig#L12429

See the test case in #179088 which is from our compiler-rt implementation.

It's not a regression, but it would be unfortunate if we had to wait another whole release cycle to be able to fix our MIPS ABI conformance.

[c-rhodes]

we have no maintainer for this backend so adding @nikic as last resort, although the original PR wasn't reviewed, and the author isn't respondent to this and another PR #186008 they're trying to backport, so my view would be to not merge.