[llvm-commits] [PATCH, RFC] Medium code model support for 64-bit PowerPC

[Bill Schmidt]

On Thu, 2012-11-15 at 14:53 -0600, Will Schmidt wrote:
> On Thu, 2012-11-15 at 10:51 -0600, Bill Schmidt wrote:
> > 
> >            From: 
> > Bill Schmidt
> > <wschmidt at linux.vnet.ibm.com>
> >              To: 
> > llvm-commits at cs.uiuc.edu
> >         Subject: 
> > [llvm-commits]
> > [PATCH,  RFC]
> > Medium code model
> > support for
> > 64-bit PowerPC
> >            Date: 
> >  Thu, 15 Nov 2012
> >    10:51:49 -0600
> > 
> > 
> >                                                                 Hello,
> 
> 
> Hi!    ;-) 
> 
> Awesome writeup, just a couple questions and comments throughout. 
> 
> 
> > Note that for LDtocL, we generate a new form of LD called LDrs.  This
> 
> I initially had a question on the LDrs , but your comment in-code
> answered it.   (... LDrs, which is a form of LD with the offset
> specified by a SymbolLo ...).

OK, I will make this more clear in the final commit note.

> 
> 
> 
> > Index: lib/Target/PowerPC/PPCInstr64Bit.td
> > ===================================================================
> 
> > @@ -625,6 +629,12 @@ let canFoldAsLoad = 1, PPC970_Unit = 2 in {
> >  def LD   : DSForm_1<58, 0, (outs G8RC:$rD), (ins memrix:$src),
> >                      "ld $rD, $src", LdStLD,
> >                      [(set G8RC:$rD, (load ixaddr:$src))]>, isPPC64;
> > +def LDrs : DSForm_1<58, 0, (outs G8RC:$rD), (ins memrs:$src),
> > +                    "ld $rD, $src", LdStLD,
> > +                    []>, isPPC64;
> > +// The following three definitions are selected for small code model
> > only.
> > +// Otherwise, we need to create two instructions to form a 32-bit
> > offset,
> > +// so we have a custom matcher for TOC_ENTRY in
> > PPCDAGToDAGIsel::Select().
> 
> ^^ re this comment, i don't see any changes to the following
> definitions, so this is just an informational for existing code? 
> 

That's correct.  It seemed to me that somebody coming upon those without
previous knowledge could easily be misled into thinking they were always
matched during Select() -- which they were, prior to this patch.

> 
> 
> > +/// lookUpOrCreateTOCEntry -- Given a symbol, look up whether a TOC
> > entry
> > +/// exists for it.  If not, create one.  Then return a symbol that
> > references
> > +/// the TOC entry.
> > +MCSymbol *PPCAsmPrinter::lookUpOrCreateTOCEntry(MCSymbol *Sym) {
> > +
> > +  MCSymbol *&TOCEntry = TOC[Sym];
> > +
> > +  // To avoid name clash check if the name already exists.
> > +  while (TOCEntry == 0) {
> > +    if (OutContext.LookupSymbol(Twine(MAI->getPrivateGlobalPrefix())
> > +
> > +                                "C" + Twine(TOCLabelID++)) == 0) {
> > +      TOCEntry = GetTempSymbol("C", TOCLabelID);
> > +    }
> > +  }
> > +
> > +  return TOCEntry;
> > +}
> 
> Nice break-out.  :-)
> 
> > +
> > +    // Change the opcode to LDrs, which is a form of LD with the
> > offset
> > +    // specified by a SymbolLo.  If the global address is external,
> 
> ^ It was useful to me to also have LDrs described in the patch
> description per my earlier comment.

Yep.

> 
> 
> > Index: lib/Target/PowerPC/PPCISelDAGToDAG.cpp
> > ===================================================================
> > --- lib/Target/PowerPC/PPCISelDAGToDAG.cpp      (revision 168008)
> > +++ lib/Target/PowerPC/PPCISelDAGToDAG.cpp      (working copy)
> > @@ -25,6 +25,7 @@
> >  #include "llvm/Constants.h"
> >  #include "llvm/Function.h"
> >  #include "llvm/GlobalValue.h"
> > +#include "llvm/GlobalVariable.h"
> >  #include "llvm/Intrinsics.h"
> >  #include "llvm/Support/Debug.h"
> >  #include "llvm/Support/MathExtras.h"
> > @@ -1268,7 +1269,49 @@ SDNode *PPCDAGToDAGISel::Select(SDNode *N) {
> >                                             Chain), 0);
> >      return CurDAG->SelectNodeTo(N, Reg, MVT::Other, Chain);
> >    }
> > +  case PPCISD::TOC_ENTRY: {
> > +    assert (PPCSubTarget.isPPC64() && "Only supported for 64-bit
> > ABI");
> 
> 
> ^ Should that be "64-bit Elf Linux ABI" ? 

Well, that's a question that I have wrestled with a bit.  I believe the
answer is no.  I don't know of any reason why Darwin code can't use the
same code generation for medium code model, but if someone does know of
such a reason I'd be happy to know about it, and we can certainly
restrict its use to ELF Linux.  Of course, the patch will have no effect
on any code that doesn't use medium code model, and currently there is
no medium code model for any PPC subtarget.

Now, I do think that we will only set medium code model as the default
(eventually) for PPC64 ELF Linux.  That will ensure that Darwin code
will only use medium code model if it is specifically requested.  So I
believe nothing is broken in that regard as the patch stands.

> 
> 
> Thanks, 
> -Will
> 
> 

Thanks for the review!

Bill