[RegisterPressure] NFC: Clean up RP handling for instructions with overlapping Def/Use

[jrbyrnes]

The current RP handling for uses of an MI that overlap with defs is confusing and unnecessary. Moreover, the lane masks do not accurately model the liveness behavior of the subregs. This cleans things up a bit and more accurately models subreg lane liveness by sinking the use handling into subsent Uses loop.

The effect of this PR is to replace

A. increaseRegPressure(Reg, LiveAfter, ~LiveAfter & LiveBefore)

with

B. increaseRegPressure(Reg, LiveAfter, LiveBefore)

Note that A (Defs loop) and B (Uses loop) have different definitions of LiveBefore

A. LiveBefore = (LiveAfter & ~DefLanes) | UseLanes

and

B. LiveBefore = LiveAfter | UseLanes

Also note, increaseRegPressure will exit if PrevMask (LiveAfter for both A/B) has any active lanes, thus these calls will only have an effect if LiveAfter is 0.

A. NewMask = ~LiveAfter & ((LiveAfter & ~DefLanes) | UseLanes) => (1 & UseLanes) => UseLanes = (0 | UseLanes) => (LiveAfter | UseLanes) = NewMask B.

[llvmbot]

@llvm/pr-subscribers-llvm-regalloc

Author: Jeffrey Byrnes (jrbyrnes)

Changes

The current RP handling for uses of an MI that overlap with defs is confusing and unnecessary. Moreover, it is incorrect if the RP tracked register by subreg / lane. This cleans it up a bit by snking the use handling into the subsequent use loop (which has more accurate handling if we were to track RP by subreg).

The effect of this PR is to replace

A. increaseRegPressure(Reg, LiveAfter, ~LiveAfter & LiveBefore)

with

B. increaseRegPressure(Reg, LiveAfter, LiveBefore)

Note that A and B have different definitions of LiveBefore

A. LiveBefore = (LiveAfter & ~DefLanes) | UseLanes

and

B. LiveBefore = LiveAfter | UseLanes

Also note, increaseRegPressure will exit if PrevMask (LiveAfter for both cases) has any active lanes, thus these calls will only have effect if LiveAfter is 0.

A. NewMask = ~LiveAfter & ((LiveAfter & ~DefLanes) | UseLanes) => (1 & UseLanes) => UseLanes = (0 | UseLanes) => (LiveAfter | UseLanes) = NewMask B.

---

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

1 Files Affected:

• (modified) llvm/lib/CodeGen/RegisterPressure.cpp (+2-8)

diff --git a/llvm/lib/CodeGen/RegisterPressure.cpp b/llvm/lib/CodeGen/RegisterPressure.cpp
index 59a1911555e9cd..a517cb9631556e 100644
--- a/llvm/lib/CodeGen/RegisterPressure.cpp
+++ b/llvm/lib/CodeGen/RegisterPressure.cpp
@@ -1060,18 +1060,12 @@ void RegPressureTracker::bumpUpwardPressure(const MachineInstr *MI) {
     LaneBitmask LiveBefore = (LiveAfter & ~DefLanes) | UseLanes;
 
     // There may be parts of the register that were dead before the
-    // instruction, but became live afterwards. Similarly, some parts
-    // may have been killed in this instruction.
+    // instruction, but became live afterwards.
     decreaseRegPressure(Reg, LiveAfter, LiveAfter & LiveBefore);
-    increaseRegPressure(Reg, LiveAfter, ~LiveAfter & LiveBefore);
   }
-  // Generate liveness for uses.
+  // Generate liveness for uses. Also handle any uses which overlap with defs.
   for (const RegisterMaskPair &P : RegOpers.Uses) {
     Register Reg = P.RegUnit;
-    // If this register was also in a def operand, we've handled it
-    // with defs.
-    if (getRegLanes(RegOpers.Defs, Reg).any())
-      continue;
     LaneBitmask LiveAfter = LiveRegs.contains(Reg);
     LaneBitmask LiveBefore = LiveAfter | P.LaneMask;
     increaseRegPressure(Reg, LiveAfter, LiveBefore);

[jayfoad]

This cleans things up a bit and more accurately models subreg lane liveness

This suggests that it is not NFC? Is there any way of testing this?

[qcolombet]

This cleans things up a bit and more accurately models subreg lane liveness

This suggests that it is not NFC? Is there any way of testing this?

I think you're right that the wording suggests that, but the "proof" that @jrbyrnes wrote in the commit message shows that this is NFC.
I haven't checked yet if the assumption that LiveAfter is 0 is true though.

[qcolombet]

I haven't checked yet if the assumption that LiveAfter is 0 is true though.

I confirm that this is true. If we already counted the regunit there's no point in adding it again, hence the behavior that @jrbyrnes described.
(Note that I'm neither approving or denying the PR. @jayfoad if you feel comfortable reviewing go for it! I haven't yet; only the commit message :))

[jrbyrnes]

This suggests that it is not NFC?

Well, yes and no. With the way things currently are, this change has no impact as the versions of the code are logically equivalent. This is because the generic trackers do not track by subreg.

Since the RP tracker tracks by whole reg, if (when increasing RP) any lane in the register was previously live, then we do not modify RP (even if Width - 1 Lanes just become live). Thus, if the PreviousMask had any live lanes, we exit from increaseRegPressure since the whole reg is already live. Due to this exit, the code versions are logically equivalent (as @qcolombet confirmed)

This cleans things up a bit and more accurately models subreg lane liveness

However, if we were to track RP by subreg (instead of whole reg), we would be interested in looking at the delta between the previous mask and the new mask in terms of individual lanes. This implies that PrevMask would model the state of live lanes before the instruction, and NewMask would model the state of live lanes after the instruction.

The current call to increaseRegPressure(Reg, LiveAfter, ~LiveAfter & LiveBefore) would then be incorrect. The PrevMask (LiveAfter) and NewMask (~LiveAfter & LiveBefore) are inconsistent (no lane overlap) and don't accurately model the delta for the use lanes. In particular, the NewMask (~LiveAfter & LiveBefore) doesn't accurately model the live lanes "after" the instruction from a bottomup perspective. The NewMask in the Uses loop (LiveBefore) captures this property much more accurately.

Thus, when tracking by subreg, there are functional changes. (e.g. we need to account for this in #93090 or else RP tracking is broken).

Is there any way of testing this?

Yeah, I think it may be helpful since there are a lot of details to consider when analyzing this code. The best I can think of is a unittest which checks the masks -- I'll look into this