[PATCH] Fix FastISel bug in boolean returns for PowerPC

[Samuel F Antao]

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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140916/376b4b38/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ppcfast-isel.patch
Type: application/octet-stream
Size: 6924 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140916/376b4b38/attachment.obj>