Page MenuHomePhabricator

AMDGPU: fix for SIRegisterInfo::isVGPR() crash

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



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

Space before "?".

arsenm requested changes to this revision.Feb 19 2018, 9:47 AM
arsenm added inline comments.

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

Then we should have all the reg classes listed here:

const TargetRegisterClass *SIRegisterInfo::getPhysRegClass(unsigned Reg) const {

  static const TargetRegisterClass *const BaseClasses[] = {

  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

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

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

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


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


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