[CodeGen] Ensure clearRegisterKills clears inside bundles. #149177

rj-jesus · 2025-07-16T20:17:47Z

When clearing register kills, ensure we do so inside a bundle too. This came up in AArch64LdStOpt when we attempt to clear kill flags that affect the first store's register. If the kill happens in a bundle, we were only clearing it in the bundle operands, but not in the constituent instructions. This was leading to a verification error, for example: https://godbolt.org/z/WjhEWGcPW.

Fixes #149092.

llvmbot · 2025-07-16T20:18:17Z

@llvm/pr-subscribers-backend-aarch64

Author: Ricardo Jesus (rj-jesus)

Changes

When clearing register kills, ensure we do so inside a bundle too. This came up in AArch64LdStOpt where we attempt to clear kill flags that affect the first store's register. If the kill happens in a bundle, we were only clearing it in the bundle operands, but not in the constituent instructions. This was leading to a verification error, for example: https://godbolt.org/z/WjhEWGcPW.

Fixes #149092.

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

2 Files Affected:

(modified) llvm/lib/CodeGen/MachineInstr.cpp (+4-4)
(modified) llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir (+20)

diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
index da3665b3b6a0b..a966add78a97e 100644
--- a/llvm/lib/CodeGen/MachineInstr.cpp
+++ b/llvm/lib/CodeGen/MachineInstr.cpp
@@ -2177,12 +2177,12 @@ void MachineInstr::clearRegisterKills(Register Reg,
                                       const TargetRegisterInfo *RegInfo) {
   if (!Reg.isPhysical())
     RegInfo = nullptr;
-  for (MachineOperand &MO : operands()) {
-    if (!MO.isReg() || !MO.isUse() || !MO.isKill())
+  for (MIBundleOperands O(*this); O.isValid(); ++O) {
+    if (!O->isReg() || !O->isUse() || !O->isKill())
       continue;
-    Register OpReg = MO.getReg();
+    Register OpReg = O->getReg();
     if ((RegInfo && RegInfo->regsOverlap(Reg, OpReg)) || Reg == OpReg)
-      MO.setIsKill(false);
+      O->setIsKill(false);
   }
 }
 
diff --git a/llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir b/llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir
index 49453bc178914..28c5220642ec5 100644
--- a/llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir
+++ b/llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir
@@ -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>))

github-actions · 2025-07-16T20:19:21Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

AZero13 · 2025-07-16T21:09:01Z

Wonder if the other functions should also operate on bundles or if just this one needs to

davemgreen · 2025-07-17T10:27:44Z

Ah I see this was only tangentially related to #134068 - it just made it mode likely to trigger.

I fixed #146415 recently for a similar issue, but there it used a different iterator type to make sure the instruction in the bundle were also visited, not skipped over.

Perhaps @arsenm knows if this sounds OK for the generic MachineInstr::clearRegisterKills? It seems sensible to me but we could just try and make sure the the AArch64LoadStore optimizer visited the instruction in the bundle too.

rj-jesus · 2025-07-17T12:35:24Z

Yeah, I think so, due to the MOVPRFX bundles.

Would you rather I move this over to AArch64LoadStoreOpt (perhaps with a different iterator for the instructions)? It looks like it might be helpful in AArch64LoadStoreOpt::promoteLoadFromStore too, although we might need a contrived and probably unrealistic example to trigger it (presumably a store's register could be killed in a bundle before a promotable load, but I don't think we'd generate such code?).

rj-jesus · 2025-07-17T12:41:30Z

Wonder if the other functions should also operate on bundles or if just this one needs to

I'm not sure, bundle iterators are already used elsewhere (for example in LiveRegUnits::accumulateUsedDefed), but it might be that there are a few places where they are missed or where different strategies are used (for example as @davemgreen suggested).

arsenm

For the various MachineInstr property checks, there is the QueryType for how to handle the bundle, which all default to AnyInBundle. The more sophisticated checks seem to have never been updated to do anything with bundle. I guess doing this in all of these type functions is the most internally consistent way to do it

rj-jesus · 2025-07-18T09:57:14Z

Hi @arsenm, when you write:

I guess doing this in all of these type functions (...)

By this, are you referring to updating "the more sophisticated checks (that) seem to have never been updated to do anything with bundle", or are you referring to adding something like AnyInBundle to clearRegisterKills, and potentially other such functions, to control how they handle a bundle? (I'm sorry if this is very obvious, but I just got confused interpreting the path you're suggesting.)

arsenm · 2025-07-18T10:06:28Z

By this, are you referring to updating "the more sophisticated checks (that) seem to have never been updated to do anything with bundle", or are you referring to adding something like AnyInBundle to clearRegisterKills, and potentially other such functions, to control how they handle a bundle? (I'm sorry if this is very obvious, but I just got confused interpreting the path you're suggesting.)

Yes, something like that. All of these register based functions, e.g. substituteRegister, clearRegisterKills, addRegisterDead and so on. They possibly should have a bundle query, like the predicate functions.

rj-jesus · 2025-07-18T10:17:59Z

Yes, something like that. All of these register based functions, e.g. substituteRegister, clearRegisterKills, addRegisterDead and so on. They possibly should have a bundle query, like the predicate functions.

Thanks very much, that sounds good to me. In that case, if you think this is a reasonable approach, I'll update this PR as you're suggesting for clearRegisterKills so that we can progress the linked issue, and create a separate PR to extend this to the remaining functions. Does this sound okay to you?

arsenm · 2025-07-18T10:29:21Z

Yes

rj-jesus · 2025-07-21T09:16:51Z

llvm/include/llvm/CodeGen/MachineInstr.h

-    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


These formatting changes were made to avoid code formatter errors.

rj-jesus · 2025-07-21T09:21:09Z

llvm/include/llvm/CodeGen/MachineInstr.h

@@ -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);


@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.

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)

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.

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).

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

rj-jesus · 2025-07-28T09:52:06Z

Ping

llvm/lib/CodeGen/MachineInstr.cpp

davemgreen · 2025-08-05T06:35:10Z

llvm/include/llvm/CodeGen/MachineInstr.h

@@ -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);


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)

As an alternative to #149177, iterate through all instructions in `AArch64LoadStoreOptimizer`.

rj-jesus · 2025-08-13T09:46:04Z

I'm closing this PR since the original motivating issue was instead fixed via #152994.

[CodeGen] Ensure clearRegisterKills works inside bundles.

dfa67db

rj-jesus requested review from arsenm, davemgreen and paulwalker-arm July 16, 2025 20:17

llvmbot added backend:AArch64 llvm:codegen labels Jul 16, 2025

rj-jesus requested a review from sjoerdmeijer July 17, 2025 07:24

arsenm reviewed Jul 18, 2025

View reviewed changes

Make clearRegisterKills bundle aware.

a3632c7

rj-jesus commented Jul 21, 2025

View reviewed changes

davemgreen reviewed Aug 5, 2025

View reviewed changes

Use Type == IgnoreBundle for non-bundle path.

f00a865

rj-jesus mentioned this pull request Aug 11, 2025

[AArch64] Fix stp kill when merging forward. #152994

Merged

rj-jesus added a commit that referenced this pull request Aug 12, 2025

[AArch64] Fix stp kill when merging forward. (#152994)

ef5e65d

As an alternative to #149177, iterate through all instructions in `AArch64LoadStoreOptimizer`.

rj-jesus closed this Aug 13, 2025

[CodeGen] Ensure clearRegisterKills clears inside bundles. #149177

[CodeGen] Ensure clearRegisterKills clears inside bundles. #149177

Uh oh!

Conversation

rj-jesus commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 16, 2025

Uh oh!

github-actions bot commented Jul 16, 2025

Uh oh!

AZero13 commented Jul 16, 2025

Uh oh!

davemgreen commented Jul 17, 2025

Uh oh!

rj-jesus commented Jul 17, 2025

Uh oh!

rj-jesus commented Jul 17, 2025

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

rj-jesus commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm commented Jul 18, 2025

Uh oh!

rj-jesus commented Jul 18, 2025

Uh oh!

arsenm commented Jul 18, 2025

Uh oh!

rj-jesus Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

rj-jesus Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

davemgreen Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

rj-jesus Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

davemgreen Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

rj-jesus Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

rj-jesus commented Jul 28, 2025

Uh oh!

Uh oh!

davemgreen Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

rj-jesus commented Aug 13, 2025

Uh oh!

Uh oh!

rj-jesus commented Jul 16, 2025 •

edited

Loading

rj-jesus commented Jul 18, 2025 •

edited

Loading