[llvm] r339350 - [MC] Remove PhysRegSize from MCRegisterClass

Fri Aug 10 02:47:59 PDT 2018

> -----Original Message-----
> From: Justin Bogner <justin at justinbogner.com> On Behalf Of Justin Bogner
> Sent: den 10 augusti 2018 09:05
> To: llvm-commits <llvm-commits at lists.llvm.org>
> Cc: Björn Pettersson A <bjorn.a.pettersson at ericsson.com>
> Subject: Re: [llvm] r339350 - [MC] Remove PhysRegSize from
> MCRegisterClass
> 
> Bjorn Pettersson via llvm-commits <llvm-commits at lists.llvm.org> writes:
> > Author: bjope
> > Date: Thu Aug  9 08:19:07 2018
> > New Revision: 339350
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=339350&view=rev
> > Log:
> > [MC] Remove PhysRegSize from MCRegisterClass
> >
> > Summary:
> > The interface to get size and spill size of a register
> > was moved from MCRegisterInfo to TargetRegisterInfo over
> > a year ago. Afaik the old interface has been around
> > to give out-of-tree targets a chance to adapt to the
> > new interface.
> 
> When you make a change like this, please actually point out what the old
> and new interface are, and how to upgrade from one to the other. My out
> of tree backend is currently broken and it's very unclear how I'm
> supposed to change my code to continue to work instead of calling
> MCRegisterInfo::getSize, based on this commit message.
> 
> My point is, "It was moved over a year ago" isn't very easy to go on,
> and it's generally better to say something like it was moved in rXYZ,
> and clients should now call foo() instead. I realize it's a bit of a
> burden, but the more details you can provide on how to upgrade the
> better, and folks with out of tree targets really appreciate it if you
> can put that extra little bit of effort in.
> 
> Thanks.

Sorry about that. I'm working on an OOT target myself, and I often
end up playing detective and thinking "if only that had been mentioned
in the commit msg". Apparently, I'm no better myself.

I gave some suggestion in https://reviews.llvm.org/D50285 about what
to migrate to.

> 
> > One problem with the old MCRegisterClass::PhysRegSize was that
> > it represented the size of a register as "size in bits" / 8.
> > So a register had to be a multiple of eight bits wide for the
> > size to be correct (and the byte size for the target needed to
> > be eight bits).
> >
> > Reviewers: kparzysz, qcolombet
> >
> > Reviewed By: kparzysz
> >
> > Subscribers: llvm-commits
> >
> > Differential Revision: https://reviews.llvm.org/D47199
> >
> > Modified:
> >     llvm/trunk/include/llvm/MC/MCRegisterInfo.h
> >     llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp
> >     llvm/trunk/utils/TableGen/RegisterInfoEmitter.cpp
> >
> > Modified: llvm/trunk/include/llvm/MC/MCRegisterInfo.h
> > URL:
> > http://llvm.org/viewvc/llvm-
> project/llvm/trunk/include/llvm/MC/MCRegisterInfo.h?rev=339350&r1=339
> 349&r2=339350&view=diff
> >
> ==========================================================
> ====================
> >
> > --- llvm/trunk/include/llvm/MC/MCRegisterInfo.h (original)
> > +++ llvm/trunk/include/llvm/MC/MCRegisterInfo.h Thu Aug  9 08:19:07
> 2018
> > @@ -41,7 +41,6 @@ public:
> >    const uint16_t RegsSize;
> >    const uint16_t RegSetSize;
> >    const uint16_t ID;
> > -  const uint16_t PhysRegSize;
> >    const int8_t CopyCost;
> >    const bool Allocatable;
> >
> > @@ -80,11 +79,6 @@ public:
> >      return contains(Reg1) && contains(Reg2);
> >    }
> >
> > -  /// Return the size of the physical register in bytes.
> > -  unsigned getPhysRegSize() const { return PhysRegSize; }
> > -  /// Temporary function to allow out-of-tree targets to switch.
> > -  unsigned getSize() const { return getPhysRegSize(); }
> > -
> >    /// getCopyCost - Return the cost of copying a value between two
> registers in
> >    /// this class. A negative number means the register class is very
> expensive
> >    /// to copy e.g. status flag register classes.
> >
> > Modified: llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp
> > URL: http://llvm.org/viewvc/llvm-
> project/llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp?rev=339350&r1=33934
> 9&r2=339350&view=diff
> >
> ==========================================================
> ====================
> > --- llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp Thu Aug  9 08:19:07 2018
> > @@ -388,8 +388,7 @@ bool TargetInstrInfo::getStackSlotRange(
> >      return true;
> >    }
> >    unsigned BitSize = TRI->getSubRegIdxSize(SubIdx);
> > -  // Convert bit size to byte size to be consistent with
> > -  // MCRegisterClass::getSize().
> > +  // Convert bit size to byte size.
> >    if (BitSize % 8)
> >      return false;
> >
> >
> > Modified: llvm/trunk/utils/TableGen/RegisterInfoEmitter.cpp
> > URL: http://llvm.org/viewvc/llvm-
> project/llvm/trunk/utils/TableGen/RegisterInfoEmitter.cpp?rev=339350&r1=
> 339349&r2=339350&view=diff
> >
> ==========================================================
> ====================
> > --- llvm/trunk/utils/TableGen/RegisterInfoEmitter.cpp (original)
> > +++ llvm/trunk/utils/TableGen/RegisterInfoEmitter.cpp Thu Aug  9 08:19:07
> 2018
> > @@ -1075,14 +1075,10 @@ RegisterInfoEmitter::runMCDesc(raw_ostre
> >
> >    for (const auto &RC : RegisterClasses) {
> >      assert(isInt<8>(RC.CopyCost) && "Copy cost too large.");
> > -    uint32_t RegSize = 0;
> > -    if (RC.RSI.isSimple())
> > -      RegSize = RC.RSI.getSimple().RegSize;
> >      OS << "  { " << RC.getName() << ", " << RC.getName() << "Bits, "
> >         << RegClassStrings.get(RC.getName()) << ", "
> >         << RC.getOrder().size() << ", sizeof(" << RC.getName() << "Bits), "
> >         << RC.getQualifiedName() + "RegClassID" << ", "
> > -       << RegSize/8 << ", "
> >         << RC.CopyCost << ", "
> >         << ( RC.Allocatable ? "true" : "false" ) << " },\n";
> >    }
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits