[PATCH] Position Independ32-bit PowerPC ELF ent Code

Fri Jul 18 16:38:51 PDT 2014

----- 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 5:14:37 PM
> Subject: Re: [PATCH] 32-bit PowerPC ELF Position Independent Code
> 
> Hal,
> 
> D'oh, I manually ran the tests individually, rather than through the
> framework.  Here's a patch that actually works.

r213427, thanks!

 -Hal

> 
> - Justin
> 
> On Fri, Jul 18, 2014 at 1:16 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > Justin,
> >
> > I just tried to apply this patch but there is a regression test
> > failure: Can you reproduce this?
> >
> >  -Hal
> >
> > FAIL: LLVM :: CodeGen/PowerPC/stack-realign.ll (2902 of 11227)
> > ******************** TEST 'LLVM ::
> > CodeGen/PowerPC/stack-realign.ll' FAILED ********************
> > Script:
> > --
> > /llvm-trunk-writable-build/Release+Asserts/bin/llc
> > -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 <
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll |
> > /llvm-trunk-writable-build/Release+Asserts/bin/FileCheck
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll
> > /llvm-trunk-writable-build/Release+Asserts/bin/llc
> > -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 -disable-fp-elim <
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll |
> > /llvm-trunk-writable-build/Release+Asserts/bin/FileCheck
> > -check-prefix=CHECK-FP
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll
> > /llvm-trunk-writable-build/Release+Asserts/bin/llc
> > -mtriple=powerpc-unknown-linux-gnu -disable-fp-elim <
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll |
> > /llvm-trunk-writable-build/Release+Asserts/bin/FileCheck
> > -check-prefix=CHECK-32
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll
> > /llvm-trunk-writable-build/Release+Asserts/bin/llc
> > -mtriple=powerpc-unknown-linux-gnu -disable-fp-elim <
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll |
> > /llvm-trunk-writable-build/Release+Asserts/bin/FileCheck
> > -check-prefix=CHECK-32-PIC
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll
> > --
> > Exit Code: 1
> >
> > Command Output (stderr):
> > --
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll:89:21:
> > error: expected string not found in input
> > ; CHECK-32-PIC-DAG: stw 29, -12(1)
> >                     ^
> > <stdin>:7:2: note: scanning from here
> >  .cfi_startproc
> >  ^
> > <stdin>:10:2: note: possible intended match here
> >  stw 31, -4(1)
> >  ^
> > /src/llvm-trunk-writable/test/CodeGen/PowerPC/stack-realign.ll:145:21:
> > error: expected string not found in input
> > ; CHECK-32-PIC-DAG: stw 29, -12(1)
> >                     ^
> > <stdin>:49:2: note: scanning from here
> >  .cfi_startproc
> >  ^
> > <stdin>:53:2: note: possible intended match here
> >  stw 31, -4(1)
> >  ^
> >
> > --
> >
> > ********************
> > Testing Time: 68.39s
> > ********************
> > Failing Tests (1):
> >     LLVM :: CodeGen/PowerPC/stack-realign.ll
> >
> >   Expected Passes    : 11101
> >   Expected Failures  : 90
> >   Unsupported Tests  : 35
> >   Unexpected Failures: 1
> > make[1]: *** [check-local] Error 1
> >
> >
> > ----- 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 2:04:22 AM
> >> Subject: Re: [PATCH] 32-bit PowerPC ELF Position Independent Code
> >>
> >> Hi Hal,
> >>
> >> Here's the latest patch with whitespace and line fixes.  Let me
> >> know
> >> if
> >> there are any other issues, cosmetic or otherwise.  Some of the
> >> whitespace I think looks a little weird, because I tried to follow
> >> indenting rules to line up function arguments, as it appears
> >> llvm-style
> >> is, and it came out kind of weird with outdents due to nested
> >> function
> >> calls.
> >>
> >> - Justin
> >>
> >> On Fri, 18 Jul 2014 01:09:52 -0500
> >> Hal Finkel <hfinkel at anl.gov> wrote:
> >>
> >> > 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
> 

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