AMDGPU/SI: Only look at live out SGPR defs

[arsenm]

Summary

When trying to fix SGPR live ranges, skip defs that are
killed in the same block as the def. I don't think
we need to worry about these cases as long as the
live ranges of the SGPRs in dominating blocks are
correct.

This reduces the number of elements the second
loop over the function needs to look at, and makes
it generally easier to understand. The second loop
also only considers if the live range is live
in to a block, which logically means it
must have been live out from another.

[• tstellarAMD]

When trying to fix SGPR live ranges, skip defs that are
killed in the same block as the def. I don't think
we need to worry about these cases as long as the
live ranges of the SGPRs in dominating blocks are
correct.

I think this will break this case:

def vreg0 <- assigned sgpr0

if:
def vreg1 <- assigned sgpr0
kill vreg1 <- assigned sgpr0
br endif

else:
use vreg0 <- assigned sgpr0
br endif

endif:
...

[• tstellarAMD]

It doesn't look like phab picked up your email response, so I'm reproducing it here:

I’m not sure I see why. vreg0 is live out of the entry block, so it will have it’s range extended until after the endif, so I don’t think it can be assigned to the same register as vreg1

Ok, so you are saying this won't be a problem as long as we correctly extend the live range of vreg0. That makes sense. I would like to test this patch before it is committed.

[arsenm]

In D11880#220688, @tstellarAMD wrote:

It doesn't look like phab picked up your email response, so I'm reproducing it here:

I’m not sure I see why. vreg0 is live out of the entry block, so it will have it’s range extended until after the endif, so I don’t think it can be assigned to the same register as vreg1

Ok, so you are saying this won't be a problem as long as we correctly extend the live range of vreg0. That makes sense. I would like to test this patch before it is committed.

In this example the live range of vreg1 wouldn't have been changed anyway because the if block only has one successor. I don't think this patch actually changes the insertion behavior.

[• tstellarAMD]

LGTM.

[arsenm]

r245150

lib/Target/AMDGPU/SIFixSGPRLiveRanges.cpp

bool SIFixSGPRLiveRanges::runOnMachineFunction(MachineFunction &MF) {
   // First pass, collect all live intervals for SGPRs
   for (const MachineBasicBlock &MBB : MF) {
     for (const MachineInstr &MI : MBB) {
       for (const MachineOperand &MO : MI.defs()) {
         if (MO.isImplicit())
           continue;
         unsigned Def = MO.getReg();
         if (TargetRegisterInfo::isVirtualRegister(Def)) {
-          if (TRI->isSGPRClass(MRI.getRegClass(Def)))
-            SGPRLiveRanges.push_back(
-                std::make_pair(Def, &LIS->getInterval(Def)));
+          if (TRI->isSGPRClass(MRI.getRegClass(Def))) {
+            // Only consider defs that are live outs. We don't care about def /
+            // use within the same block.
+            LiveRange &LR = LIS->getInterval(Def);
+            if (LIS->isLiveOutOfMBB(LR, &MBB))
+              SGPRLiveRanges.push_back(std::make_pair(Def, &LR));
+          }
         } else if (TRI->isSGPRClass(TRI->getPhysRegClass(Def))) {
             SGPRLiveRanges.push_back(
                 std::make_pair(Def, &LIS->getRegUnit(Def)));
         }
       }
     }
   }