[PATCH] Fix FastISel bug in boolean returns for PowerPC

[Samuel F Antao]

Hi Hal,

Got commit access in the meantime. Patch committed as r217993

Thanks again!
Samuel

2014-09-16 14:54 GMT-04:00 Samuel F Antao <sfantao at us.ibm.com>:

> Hi Hal,
>
> Thanks for reviewing the patch. Find attached the new patch addressing
> your concerns.
>
> Hal Finkel <hfinkel at anl.gov> wrote on 09/16/2014 01:49:50 AM:
>
> > From: Hal Finkel <hfinkel at anl.gov>
> > To: Samuel F Antao/Watson/IBM at IBMUS
> > Cc: <llvm-commits at cs.uiuc.edu>
> > Date: 09/16/2014 01:49 AM
> > Subject: Re: [PATCH] Fix FastISel bug in boolean returns for PowerPC
> >
> > Hi Samuel,
> >
> > Thanks for working on this!
> >
> > +      switch (VA.getLocInfo()) {
> > +        default:
> > +          llvm_unreachable("Unknown loc info!");
> > +          break;
> > +        case CCValAssign::Full:
> > +        case CCValAssign::AExt:
> > +        case CCValAssign::ZExt:
> > +          SrcReg = PPCMaterializeInt(C, MVT::i64, false);
> > +          break;
> > +        case CCValAssign::SExt:
> > +          SrcReg = PPCMaterializeInt(C, MVT::i64, true);
> > +          break;
> > +      }
> >
> > Seems like this could just be:
> >
> >   SrcReg = PPCMaterializeInt(C, MVT::i64,
> >                              VA.getLocInfo() == CCValAssign::SExt);
> >
>
> Ok, I refactored the code as you suggested.
>
> > +define zeroext i1 @rettrue() nounwind uwtable ssp {
> > +entry:
> > +; ELF64: rettrue
> > +; ELF64: li 3, 1
> > +; ELF64: blr
> > +  ret i1 true
> > +}
> >
> > Each of these tests should use a CHECK-LABEL marker when matching
> > the test function name for better error recovery in the reporter
> > should something fail. Like this:
> >
> > +define zeroext i1 @rettrue() nounwind uwtable ssp {
> > +entry:
> > +; ELF64-LABEL: @rettrue
> > +; ELF64: li 3, 1
> > +; ELF64: blr
> > +  ret i1 true
> > +}
> >
>
> I have modified the testcase to use the ELF64-LABEL directive for all
> functions.
> I am using rettrue instead of @rettrue in the patch. I noticed that
> @rettrue is just a comment in the asm file
> and I was unsure how stable that is supposed to be. I can still change
> that to @rettrue if you want me to.
>
>
> > Otherwise, LGTM. Do you have commit access now?
>
> I do not have commit privileges. Do I need to take any action to request
> access or is someone going to commit the patch for me?
>
>
> >
> >  -Hal
> >
> > ----- Original Message -----
> > > From: "Samuel F Antao" <sfantao at us.ibm.com>
> > > To: llvm-commits at cs.uiuc.edu
> > > Sent: Wednesday, September 10, 2014 6:32:19 PM
> > > Subject: [PATCH] Fix FastISel bug in boolean returns for PowerPC
> > >
> > >
> > >
> > >
> > > Hi all,
> > >
> > > For PPC targets, FastISel does not take the sign extension
> > > information into account when selecting return instructions whose
> > > operands are constants. A consequence of this is that the return of
> > > boolean values is not correct. E.g. the following program would
> > > (erroneously) return 0:
> > >
> > > bool T(){
> > > return true;
> > > }
> > >
> > > bool F(){
> > > return !T();
> > > }
> > >
> > > inr main(){
> > > if(F())
> > > return 0;
> > > else
> > > return 1;
> > > }
> > >
> > > The reason the output of this program is wrong is that ‘true’ sign is
> > > sign to 64-bit (0xfff..fff) and the negation only flips the least
> > > significant bit. This code pattern is used in some Clang’s unit
> > > tests, and this is the reason they are failing for PowerPC when
> > > building clang with the latest clang.
> > >
> > > This patches fixes the problem by evaluating the sign extension
> > > information also for constants, forwarding this information to
> > > PPCMaterializeInt which takes this information to drive the sign
> > > extension during the materialization.
> > >
> > >
> > > The modified files are the following:
> > >
> > > lib/Target/PowerPC/PPCFastISel.cpp
> > > test/CodeGen/PowerPC/fast-isel-ret.ll
> > >
> > > Thanks,
> > >
> > > Sam
> > >
> > > (See attached file: llvm_fsel_ret.patch)
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > >
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> >
>
> Thanks again!
> Samuel
>
> *(See attached file: ppcfast-isel.patch)*
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140917/a2c2c1ee/attachment.html>