Skip to content

[CodeGen] Ensure clearRegisterKills clears inside bundles. #149177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[CodeGen] Ensure clearRegisterKills clears inside bundles. #149177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dfa67db
[CodeGen] Ensure clearRegisterKills works inside bundles.
rj-jesus Jul 16, 2025
a3632c7
Make clearRegisterKills bundle aware.
rj-jesus Jul 18, 2025
f00a865
Use Type == IgnoreBundle for non-bundle path.
rj-jesus Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions llvm/include/llvm/CodeGen/MachineInstr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,9 +880,9 @@ class MachineInstr
/// queries but they are bundle aware.

enum QueryType {
IgnoreBundle, // Ignore bundles
AnyInBundle, // Return true if any instruction in bundle has property
AllInBundle // Return true if all instructions in bundle have property
IgnoreBundle, // Ignore bundles
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These formatting changes were made to avoid code formatter errors.

AnyInBundle, // Check/update property for any instruction in bundle
AllInBundle // Check/update property for all instructions in bundle
};

/// Return true if the instruction (or in the case of a bundle,
Expand Down Expand Up @@ -1700,7 +1700,8 @@ class MachineInstr
/// Clear all kill flags affecting Reg. If RegInfo is provided, this includes
/// all aliasing registers.
LLVM_ABI void clearRegisterKills(Register Reg,
const TargetRegisterInfo *RegInfo);
const TargetRegisterInfo *RegInfo,
QueryType Type = AllInBundle);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arsenm For now I've reused the pre-existing QueryType. If you think it would be better to define a separate type for updates, please let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the default be IgnoreBundle to match the previous behaviour? (And maybe that would help for compile-time, I'm not sure how much it makes a difference)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go that route, would we then override the default in AArch64LoadStoreOptimizer?

I'm happy with that as it solves the direct issue in #149092, but I would assume that not clearing kills in bundles could be an issue elsewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code calling clearRegisterKills already iterates over all instructions they might be fine already. It's hard to tell from the code which is which, but I think the Arm uses are probably OK (there are many more bundles used there, they are rarer on AArch64 before movprfx).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks for the suggestion. I'll open a separate PR to iterate through all instructions in AArch64LoadStoreOptimizer instead.


/// We have determined MI defined a register without a use.
/// Look for the operand that defines it and mark it as IsDead. If
Expand Down
25 changes: 17 additions & 8 deletions llvm/lib/CodeGen/MachineInstr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2174,16 +2174,25 @@ bool MachineInstr::addRegisterKilled(Register IncomingReg,
}

void MachineInstr::clearRegisterKills(Register Reg,
const TargetRegisterInfo *RegInfo) {
const TargetRegisterInfo *RegInfo,
QueryType Type) {
if (!Reg.isPhysical())
RegInfo = nullptr;
for (MachineOperand &MO : operands()) {
if (!MO.isReg() || !MO.isUse() || !MO.isKill())
continue;
Register OpReg = MO.getReg();
if ((RegInfo && RegInfo->regsOverlap(Reg, OpReg)) || Reg == OpReg)
MO.setIsKill(false);
}

auto clearKills = [&](auto Operands) {
for (MachineOperand &MO : Operands) {
if (!MO.isReg() || !MO.isUse() || !MO.isKill())
continue;
Register OpReg = MO.getReg();
if ((RegInfo && RegInfo->regsOverlap(Reg, OpReg)) || Reg == OpReg)
MO.setIsKill(false);
}
};

if (Type == IgnoreBundle || !isBundled() || isBundledWithPred())
clearKills(operands());
else
clearKills(mi_bundle_ops(*this));
}

bool MachineInstr::addRegisterDead(Register Reg,
Expand Down
20 changes: 20 additions & 0 deletions llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,23 @@ body: |
# CHECK: STURQi killed renamable $q1, renamable $x1, 16 :: (store (s128))
# CHECK: STURQi killed renamable $q2, renamable $x1, 48 :: (store (s128))
# CHECK: STR_ZXI killed renamable $z3, renamable $x1, 4 :: (store (<vscale x 1 x s128>))
---
name: clear-kill-in-bundle
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0, $z0

STR_ZXI $z0, $x0, 0 :: (store (<vscale x 1 x s128>))
BUNDLE implicit-def $z1, implicit-def $q1, implicit killed $z0 {
$z1 = ADD_ZZZ_D $z0, killed $z0
}
STR_ZXI renamable $z1, $x0, 1 :: (store (<vscale x 1 x s128>))

RET_ReallyLR
...
# CHECK-LABEL: name: clear-kill-in-bundle
# CHECK: BUNDLE implicit-def $z1, implicit-def $q1, implicit $z0 {
# CHECK: $z1 = ADD_ZZZ_D $z0, $z0
# CHECK: }
# CHECK: STPQi $q0, $q1, $x0, 0 :: (store (<vscale x 1 x s128>))