[PATCH] 32-bit PowerPC ELF Position Independent Code

[Hal Finkel]

Justin,

Thanks, this looks good. You have a bunch of lines that are too long (80 characters is the limit). Now, parts of that code have long-standing violations of this, but let's not make it worse. Please wrap those lines that would go over 80 characters.

 -Hal

----- Original Message -----
> From: "Justin Hibbits" <chmeeedalf at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Friday, July 18, 2014 12:31:05 AM
> Subject: Re: [PATCH] 32-bit PowerPC ELF Position Independent Code
> 
> Hi Hal,
> 
> Thanks for looking at this.  Got another one for you now.
> 
> On Wed, 16 Jul 2014 22:20:59 -0500
> Hal Finkel <hfinkel at anl.gov> wrote:
> 
> > Hi Justin,
> > 
> > Thanks for working on this!
> > 
> > As Bill said, we need test cases. In addition, a few comments:
> > 
> > +  const MCExpr *tocExpr =
> > MCBinaryExpr::CreateAdd(MCSymbolRefExpr::Create(CurrentPos,
> > OutContext),
> > 
> > +
> > MCConstantExpr::Create(0x8000, OutContext),
> > 
> > +                                                  OutContext);
> > 
> > what is 0x8000? Please provide a comment.
> > 
> >        BuildMI(FirstMBB, MBBI, dl, TII.get(PPC::MFLR),
> >        GlobalBaseReg);
> > 
> > +      if (PPCSubTarget->isTargetELF()) {
> > 
> > +        unsigned TempReg =
> > RegInfo->createVirtualRegister(&PPC::GPRCRegClass);
> > 
> > +        BuildMI(FirstMBB, MBBI, dl, TII.get(PPC::GetGBRO),
> > TempReg).addReg(GlobalBaseReg);
> > 
> > +        BuildMI(FirstMBB, MBBI, dl,
> > TII.get(PPC::UpdateGBR)).addReg(GlobalBaseReg).addReg(TempReg);
> > 
> > +        MF->getInfo<PPCFunctionInfo>()->setUsesPICBase(true);
> > 
> > It is okay here for TempReg to be R0?
> > 
> >          // automatically synthesizes these stubs.
> > 
> > -        OpFlags = PPCII::MO_DARWIN_STUB;
> > 
> > +        OpFlags = PPCII::MO_PLT_STUB;
> > 
> > is this going to break Darwin?
> > 
> >        // unless we're building with the leopard linker or later,
> > which
> > 
> >        // automatically synthesizes these stubs.
> > 
> > -      OpFlags = PPCII::MO_DARWIN_STUB;
> > 
> > +      OpFlags = PPCII::MO_PLT_STUB;
> > 
> > this too? Darwin breaking?
> > 
> > -  explicit PPCFunctionInfo(MachineFunction &MF)
> > +  explicit PPCFunctionInfo(MachineFunction &MFC)
> > ...
> > +      MF(MFC),
> > 
> > This is unnecessary; because of the way that C++ name lookup works,
> > MF(MF) will be fine here.
> > 
> > Thanks again,
> > Hal
> 
> I think I've addressed all your comments in my latest patch.  Mind
> giving it another look?
> 
> - Justin
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory