From 9b4b4a49bc01f21d3597439c671de6e332da4cca Mon Sep 17 00:00:00 2001 From: David Green Date: Mon, 20 Oct 2025 14:29:21 +0100 Subject: [PATCH 1/2] [AArch64] Copy implicit def operands when creating LDP. Otherwise we might end up with undefined register uses. Copying implicit uses can cause problems where a register is both defined and used in the same LDP, so I have not tried to add them here. Fixes #164230 --- .../AArch64/AArch64LoadStoreOptimizer.cpp | 11 ++++ llvm/test/CodeGen/AArch64/ldst-implicitop.mir | 58 +++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 llvm/test/CodeGen/AArch64/ldst-implicitop.mir diff --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp index e69fa32967a79..96998593514a8 100644 --- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp +++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp @@ -1386,6 +1386,17 @@ AArch64LoadStoreOpt::mergePairedInsns(MachineBasicBlock::iterator I, if (MOP.isReg() && MOP.isKill()) DefinedInBB.addReg(MOP.getReg()); + // Copy over any implicit-def operands. This is like MI.copyImplicitOps, but + // only copies implicit defs. + auto CopyImplicitOps = [&](MachineBasicBlock::iterator MI) { + for (const MachineOperand &MO : + llvm::drop_begin(MI->operands(), MI->getDesc().getNumOperands())) + if (MO.isReg() && MO.isImplicit() && MO.isDef()) + MIB.add(MO); + }; + CopyImplicitOps(I); + CopyImplicitOps(Paired); + // Erase the old instructions. I->eraseFromParent(); Paired->eraseFromParent(); diff --git a/llvm/test/CodeGen/AArch64/ldst-implicitop.mir b/llvm/test/CodeGen/AArch64/ldst-implicitop.mir new file mode 100644 index 0000000000000..e29b0db0eb9de --- /dev/null +++ b/llvm/test/CodeGen/AArch64/ldst-implicitop.mir @@ -0,0 +1,58 @@ +# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 6 +# RUN: llc -mtriple=aarch64-- -run-pass=aarch64-ldst-opt -verify-machineinstrs -o - %s | FileCheck %s +# Check that we copy implicit operands. +--- +name: impdef_op1 +tracksRegLiveness: true +body: | + bb.0: + liveins: $lr + ; CHECK-LABEL: name: impdef_op1 + ; CHECK: liveins: $lr + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: renamable $q5, renamable $q20 = LDPQi renamable $lr, 3, implicit-def $q4_q5 :: (load (s128)) + ; CHECK-NEXT: $q0 = ORRv16i8 $q4, killed $q4 + ; CHECK-NEXT: $q1 = ORRv16i8 $q5, killed $q5 + ; CHECK-NEXT: RET_ReallyLR + renamable $q5 = LDRQui renamable $lr, 3, implicit-def $q4_q5 :: (load (s128)) + renamable $q20 = LDRQui renamable $lr, 4 :: (load (s128)) + $q0 = ORRv16i8 $q4, killed $q4 + $q1 = ORRv16i8 $q5, killed $q5 + RET_ReallyLR +... +--- +name: impdef_op2 +body: | + bb.0: + liveins: $lr + ; CHECK-LABEL: name: impdef_op2 + ; CHECK: liveins: $lr + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: renamable $q20, renamable $q5 = LDPQi renamable $lr, 3, implicit-def $q4_q5 :: (load (s128)) + ; CHECK-NEXT: $q0 = ORRv16i8 $q4, killed $q4 + ; CHECK-NEXT: $q1 = ORRv16i8 $q5, killed $q5 + ; CHECK-NEXT: RET_ReallyLR + renamable $q20 = LDRQui renamable $lr, 3 :: (load (s128)) + renamable $q5 = LDRQui renamable $lr, 4, implicit-def $q4_q5 :: (load (s128)) + $q0 = ORRv16i8 $q4, killed $q4 + $q1 = ORRv16i8 $q5, killed $q5 + RET_ReallyLR +... +--- +name: impdef_both +body: | + bb.0: + liveins: $lr + ; CHECK-LABEL: name: impdef_both + ; CHECK: liveins: $lr + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: renamable $q5, renamable $q20 = LDPQi renamable $lr, 3, implicit-def $q4_q5, implicit-def $q20_q21 :: (load (s128)) + ; CHECK-NEXT: $q0 = ORRv16i8 $q4, killed $q4 + ; CHECK-NEXT: $q1 = ORRv16i8 $q5, killed $q5 + ; CHECK-NEXT: RET_ReallyLR + renamable $q5 = LDRQui renamable $lr, 3, implicit-def $q4_q5 :: (load (s128)) + renamable $q20 = LDRQui renamable $lr, 4, implicit-def $q20_q21 :: (load (s128)) + $q0 = ORRv16i8 $q4, killed $q4 + $q1 = ORRv16i8 $q5, killed $q5 + RET_ReallyLR +... From 152f49dddfc4a2918b24d8748764e7192992dcd0 Mon Sep 17 00:00:00 2001 From: David Green Date: Tue, 4 Nov 2025 11:29:11 +0000 Subject: [PATCH 2/2] Add an extra testcase with duplicate implicitdefs and make sure that implicit-defs are unique. --- .../AArch64/AArch64LoadStoreOptimizer.cpp | 20 ++++++++++++----- llvm/test/CodeGen/AArch64/ldst-implicitop.mir | 22 +++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp index 96998593514a8..2ab7bf19da410 100644 --- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp +++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp @@ -1387,15 +1387,23 @@ AArch64LoadStoreOpt::mergePairedInsns(MachineBasicBlock::iterator I, DefinedInBB.addReg(MOP.getReg()); // Copy over any implicit-def operands. This is like MI.copyImplicitOps, but - // only copies implicit defs. - auto CopyImplicitOps = [&](MachineBasicBlock::iterator MI) { + // only copies implicit defs and makes sure that each operand is only added + // once in case of duplicates. + auto CopyImplicitOps = [&](MachineBasicBlock::iterator MI1, + MachineBasicBlock::iterator MI2) { + SmallSetVector Ops; for (const MachineOperand &MO : - llvm::drop_begin(MI->operands(), MI->getDesc().getNumOperands())) + llvm::drop_begin(MI1->operands(), MI1->getDesc().getNumOperands())) if (MO.isReg() && MO.isImplicit() && MO.isDef()) - MIB.add(MO); + Ops.insert(MO.getReg()); + for (const MachineOperand &MO : + llvm::drop_begin(MI2->operands(), MI2->getDesc().getNumOperands())) + if (MO.isReg() && MO.isImplicit() && MO.isDef()) + Ops.insert(MO.getReg()); + for (auto Op : Ops) + MIB.addDef(Op, RegState::Implicit); }; - CopyImplicitOps(I); - CopyImplicitOps(Paired); + CopyImplicitOps(I, Paired); // Erase the old instructions. I->eraseFromParent(); diff --git a/llvm/test/CodeGen/AArch64/ldst-implicitop.mir b/llvm/test/CodeGen/AArch64/ldst-implicitop.mir index e29b0db0eb9de..34e8cf282669c 100644 --- a/llvm/test/CodeGen/AArch64/ldst-implicitop.mir +++ b/llvm/test/CodeGen/AArch64/ldst-implicitop.mir @@ -49,10 +49,32 @@ body: | ; CHECK-NEXT: renamable $q5, renamable $q20 = LDPQi renamable $lr, 3, implicit-def $q4_q5, implicit-def $q20_q21 :: (load (s128)) ; CHECK-NEXT: $q0 = ORRv16i8 $q4, killed $q4 ; CHECK-NEXT: $q1 = ORRv16i8 $q5, killed $q5 + ; CHECK-NEXT: $q2 = ORRv16i8 $q20, killed $q20 + ; CHECK-NEXT: $q3 = ORRv16i8 $q21, killed $q21 ; CHECK-NEXT: RET_ReallyLR renamable $q5 = LDRQui renamable $lr, 3, implicit-def $q4_q5 :: (load (s128)) renamable $q20 = LDRQui renamable $lr, 4, implicit-def $q20_q21 :: (load (s128)) $q0 = ORRv16i8 $q4, killed $q4 $q1 = ORRv16i8 $q5, killed $q5 + $q2 = ORRv16i8 $q20, killed $q20 + $q3 = ORRv16i8 $q21, killed $q21 + RET_ReallyLR +... +--- +name: impdef_both_same +body: | + bb.0: + liveins: $lr + ; CHECK-LABEL: name: impdef_both_same + ; CHECK: liveins: $lr + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: renamable $q5, renamable $q20 = LDPQi renamable $lr, 3, implicit-def $q4_q5 :: (load (s128)) + ; CHECK-NEXT: $q0 = ORRv16i8 $q4, killed $q4 + ; CHECK-NEXT: $q1 = ORRv16i8 $q5, killed $q5 + ; CHECK-NEXT: RET_ReallyLR + renamable $q5 = LDRQui renamable $lr, 3, implicit-def $q4_q5 :: (load (s128)) + renamable $q20 = LDRQui renamable $lr, 4, implicit-def $q4_q5 :: (load (s128)) + $q0 = ORRv16i8 $q4, killed $q4 + $q1 = ORRv16i8 $q5, killed $q5 RET_ReallyLR ...