Page MenuHomePhabricator

AMDGPU: fix for SIRegisterInfo::isVGPR() crash
ClosedPublic

Authored by alex-t on Feb 15 2018, 6:54 AM.

Details

Reviewers
rampitec
arsenm
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.

Diff Detail

Event Timeline

alex-t created this revision.Feb 15 2018, 6:54 AM
rampitec added inline comments.Feb 15 2018, 9:16 AM
lib/Target/AMDGPU/SIRegisterInfo.cpp
1487

Space before "?".

arsenm requested changes to this revision.Feb 19 2018, 9:47 AM
arsenm added inline comments.
lib/Target/AMDGPU/SIRegisterInfo.cpp
1486–1488

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

This revision now requires changes to proceed.Feb 19 2018, 9:47 AM
alex-t added inline comments.Feb 20 2018, 2:42 AM
lib/Target/AMDGPU/SIRegisterInfo.cpp
1486–1488

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 added inline comments.Feb 20 2018, 11:03 AM
lib/Target/AMDGPU/SIRegisterInfo.cpp
1486–1488

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 added inline comments.Feb 22 2018, 11:41 AM
lib/Target/AMDGPU/SIRegisterInfo.cpp
1486–1488

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 added inline comments.Feb 26 2018, 7:49 AM
lib/Target/AMDGPU/SIRegisterInfo.cpp
1486–1488

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.

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?

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

lib/Target/AMDGPU/SIRegisterInfo.cpp
1486–1488

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 updated this revision to Diff 135931.Feb 26 2018, 11:24 AM

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.

alex-t marked 2 inline comments as done.Feb 26 2018, 11:25 AM
arsenm accepted this revision.Feb 26 2018, 2:28 PM

LGTM except for minor cleanups

lib/Target/AMDGPU/SIRegisterInfo.cpp
1487

Just RC

This revision is now accepted and ready to land.Feb 26 2018, 2:28 PM
alex-t closed this revision.Sep 13 2018, 5:52 AM