AMDGPU: fix for SIRegisterInfo::isVGPR() crash

[alex-t]

Summary

SIRegisterInfo::isVGPR() calls getRegClassForReg that may return nullptr if has not found the register class.
In this case hasVGPR dereferences null pointer and crashes.

[rampitec]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1483

Space before "?".

[arsenm]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1482

This should be an assert. There is no conservative answer that make sense

[alex-t]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1482

Then we should have all the reg classes listed here:

const TargetRegisterClass *SIRegisterInfo::getPhysRegClass(unsigned Reg) const {
  assert(!TargetRegisterInfo::isVirtualRegister(Reg));

  static const TargetRegisterClass *const BaseClasses[] = {
    &AMDGPU::VGPR_32RegClass,
    &AMDGPU::SReg_32RegClass,
    &AMDGPU::VReg_64RegClass,
    &AMDGPU::SReg_64RegClass,
    &AMDGPU::VReg_96RegClass,
    &AMDGPU::VReg_128RegClass,
    &AMDGPU::SReg_128RegClass,
    &AMDGPU::VReg_256RegClass,
    &AMDGPU::SReg_256RegClass,
    &AMDGPU::VReg_512RegClass,
    &AMDGPU::SReg_512RegClass,
    &AMDGPU::SCC_CLASSRegClass,
  };

  for (const TargetRegisterClass *BaseClass : BaseClasses) {
    if (BaseClass->contains(Reg)) {
      return BaseClass;
    }
  }
  return nullptr;
}

If you return nullptr we get crash.

What about for example this Pseudo_SReg_128RegClass
This class has one register and it may be passed to the function above

[arsenm]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1482

The pseudo classes should probably be listed here, although I wouldn't expect this to be used in a context where those still exist. What context did you run into this?

[alex-t]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1482

I used isVGPR from the TargetLowering to analyse the divergence of live-ins:

if (TRI.isPhysicalRegister(Reg))
  return TRI.isVGPR(MRI, Reg);

I guess that some pseudos might be live-in and hence might happen here

[alex-t]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1482

If you consider that there should be an assert, does it mean that returning nullptr in case you could not find appropriate register class is a design flaw? Returning null assumes the check on the caller side. From the isVGPR point of view, it is okay to return "false" if we haven't found the class for register in the function where all the VGPR classes are listed.

[alex-t]

One more question - where should R600 reg classes be processed?
Should we implement getRegClass in R600RegisterInfo?
Or it's okay to handle all them in SIRegisterInfo?

[arsenm]

There's no need to handle the r600 classes, but those would go in R600RegisterInfo

lib/Target/AMDGPU/SIRegisterInfo.cpp

1482

It is possible to have a physical register without a class (I consider SCC_CLASS for example to be a hack), so null would be correct

[alex-t]

Following the discussion. The only register classes that appeared necessary according to the test coverage were added.
This preview can be the starting point for the discussion with respect to the proper approach.

[arsenm]

LGTM except for minor cleanups

lib/Target/AMDGPU/SIRegisterInfo.cpp

1483

Just RC

lib/Target/AMDGPU/SIRegisterInfo.cpp

SIRegisterInfo::getRegClassForReg(const MachineRegisterInfo &MRI,
   if (TargetRegisterInfo::isVirtualRegister(Reg))
     return  MRI.getRegClass(Reg);
 
   return getPhysRegClass(Reg);
 }
 
 bool SIRegisterInfo::isVGPR(const MachineRegisterInfo &MRI,
                             unsigned Reg) const {
-  return hasVGPRs(getRegClassForReg(MRI, Reg));
+  const TargetRegisterClass * RC = getRegClassForReg(MRI, Reg);
+  return RC? hasVGPRs(RC) : false;
 }
 
 bool SIRegisterInfo::shouldCoalesce(MachineInstr *MI,
                                     const TargetRegisterClass *SrcRC,
                                     unsigned SubReg,
                                     const TargetRegisterClass *DstRC,
                                     unsigned DstSubReg,
                                     const TargetRegisterClass *NewRC,