<div dir="ltr">Hi Hal,<div><br></div><div>Got commit access in the meantime. Patch committed as r217993</div><div><br></div><div>Thanks again!</div><div>Samuel</div></div><div class="gmail_extra"><br><div class="gmail_quote">2014-09-16 14:54 GMT-04:00 Samuel F Antao <span dir="ltr"><<a href="mailto:sfantao@us.ibm.com" target="_blank">sfantao@us.ibm.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<p><font face="sans-serif">Hi Hal,</font><br>
<br>
<font face="sans-serif">Thanks for reviewing the patch. Find attached the new patch addressing your concerns.</font><br>
<br>
<tt><font>Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>> wrote on 09/16/2014 01:49:50 AM:<br>
<br>
> From: Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></font></tt><br>
<tt><font>> To: Samuel F Antao/Watson/IBM@IBMUS</font></tt><br>
<tt><font>> Cc: <<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>></font></tt><br>
<tt><font>> Date: 09/16/2014 01:49 AM</font></tt><br>
<tt><font>> Subject: Re: [PATCH] Fix FastISel bug in boolean returns for PowerPC</font></tt><span class=""><br>
<tt><font>> <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>
</span><tt><font>Ok, I refactored the code as you suggested.</font></tt><span class=""><br>
<tt><font><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>
</span><tt><font>I have modified the testcase to use the ELF64-LABEL directive for all functions.</font></tt><br>
<tt><font>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>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><span class=""><br>
<br>
<tt><font><br>
> Otherwise, LGTM. Do you have commit access now?</font></tt><br>
<br>
</span><tt><font>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></p><div><div class="h5"><br>
<tt><font><br>
> <br>
>  -Hal<br>
> <br>
> ----- Original Message -----<br>
> > From: "Samuel F Antao" <<a href="mailto:sfantao@us.ibm.com" target="_blank">sfantao@us.ibm.com</a>><br>
> > To: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><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>
> > <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">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>
</div></div><tt><font>Thanks again!</font></tt><br>
<tt><font>Samuel</font></tt><br>
<br>
<i>(See attached file: ppcfast-isel.patch)</i><p></p></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>