Page MenuHomePhabricator

TargetRegisterInfo: Add getRegAsmName()
ClosedPublic

Authored by tstellarAMD on Dec 17 2015, 8:24 AM.

Details

Summary

The motivation for this new function is to move an invalid assumption
about the relationship between the names of register definitions in
tablegen files and their assembly names into TargetRegisterInfo, so that
we can begin working on fixing this assumption.

The current problem is that if you have a register definition in
TableGen like:

def MYReg0 : Register<"r0", 0>;

The function TargetLowering::getRegForInlineAsmConstraint() derives the
assembly name from the tablegen name: "MyReg0" rather than the given
assembly name "r0". This is working, because on most targets the
tablegen name and the assembly names are case insensitive matches for
each other (e.g. def EAX : X86Reg<"eax", ...>

getRegAsmName() will allow targets to override this default assumption and
return the correct assembly name.

Diff Detail

Event Timeline

tstellarAMD retitled this revision from to TargetRegisterInfo: Add getRegAsmName().
tstellarAMD updated this object.
tstellarAMD added a reviewer: echristo.
tstellarAMD added a subscriber: llvm-commits.
echristo edited edge metadata.Jan 7 2016, 5:53 PM

Seems like extra complexity versus just doing what the PPC port did?

Seems like extra complexity versus just doing what the PPC port did?

How did the PPC port solve this? I haven't found anything in PPCISelLowering::getRegForInlineAsmConstraint() that seems to work-around the issue.

Seems like extra complexity versus just doing what the PPC port did?

How did the PPC port solve this? I haven't found anything in PPCISelLowering::getRegForInlineAsmConstraint() that seems to work-around the issue.

They did it via this:

class PPCReg<string n> : Register<n> {
  let Namespace = "PPC";
}

// We identify all our registers with a 5-bit ID, for consistency's sake.                                                                                                           

// GPR - One of the 32 32-bit general-purpose registers                                                                                                                             
class GPR<bits<5> num, string n> : PPCReg<n> {
  let HWEncoding{4-0} = num;
}

Where it just sets the register rather than using the default. Take a look at lib/Target/PPCRegisterInfo.td.

They did it via this:

class PPCReg<string n> : Register<n> {
  let Namespace = "PPC";
}

// We identify all our registers with a 5-bit ID, for consistency's sake.                                                                                                           

// GPR - One of the 32 32-bit general-purpose registers                                                                                                                             
class GPR<bits<5> num, string n> : PPCReg<n> {
  let HWEncoding{4-0} = num;
}

Where it just sets the register rather than using the default. Take a look at lib/Target/PPCRegisterInfo.td.

I'm not sure we are talking about this same thing, because this code appears to affect the encoding value and not the assembly string.

hfinkel accepted this revision.Apr 9 2016, 9:58 AM
hfinkel added a reviewer: hfinkel.
hfinkel added a subscriber: hfinkel.

They did it via this:

class PPCReg<string n> : Register<n> {
  let Namespace = "PPC";
}

// We identify all our registers with a 5-bit ID, for consistency's sake.                                                                                                           

// GPR - One of the 32 32-bit general-purpose registers                                                                                                                             
class GPR<bits<5> num, string n> : PPCReg<n> {
  let HWEncoding{4-0} = num;
}

Where it just sets the register rather than using the default. Take a look at lib/Target/PPCRegisterInfo.td.

I'm not sure we are talking about this same thing, because this code appears to affect the encoding value and not the assembly string.

That's right, this is a different issue. PPC does have a workaround for this:

// r[0-9]+ are used, on PPC64, to refer to the corresponding 64-bit registers
// (which we call X[0-9]+). If a 64-bit value has been requested, and a
// 32-bit GPR has been selected, then 'upgrade' it to the 64-bit parent
// register.
// FIXME: If TargetLowering::getRegForInlineAsmConstraint could somehow use
// the AsmName field from *RegisterInfo.td, then this would not be necessary.
if (R.first && VT == MVT::i64 && Subtarget.isPPC64() &&
    PPC::GPRCRegClass.contains(R.first))
  return std::make_pair(TRI->getMatchingSuperReg(R.first,
                          PPC::sub_32, &PPC::G8RCRegClass),
                        &PPC::G8RCRegClass);

and I'd like to get rid of this.

This seems like a good first step; LGTM.

This revision is now accepted and ready to land.Apr 9 2016, 9:58 AM

Sure. I guess I was hoping for something else, but I don't have a strong
enough opinion when someone else really wants something

SamWot added a subscriber: SamWot.Apr 11 2016, 6:35 AM
This revision was automatically updated to reflect the committed changes.