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