<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=us-ascii"><meta name=Generator content="Microsoft Word 14 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:Helvetica;
panose-1:2 11 6 4 2 2 2 2 2 4;}
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:"\@SimSun";
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.MsoAcetate, li.MsoAcetate, div.MsoAcetate
{mso-style-priority:99;
mso-style-link:"Balloon Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:8.0pt;
font-family:"Tahoma","sans-serif";}
span.apple-converted-space
{mso-style-name:apple-converted-space;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.BalloonTextChar
{mso-style-name:"Balloon Text Char";
mso-style-priority:99;
mso-style-link:"Balloon Text";
font-family:"Tahoma","sans-serif";}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Oops, sorry for the typo. Thanks for catching it.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Weiming<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation</span><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p></o:p></span></p></div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><div><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in'><p class=MsoNormal><b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>From:</span></b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'> Quentin Colombet [mailto:qcolombet@apple.com] <br><b>Sent:</b> Thursday, December 05, 2013 5:27 PM<br><b>To:</b> Weiming Zhao<br><b>Cc:</b> llvm-commits<br><b>Subject:</b> Re: [Patch][AArch32] Fix vsel in compareinst optimization<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><div><p class=MsoNormal>Thanks for the updated patch.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>The logical changes look good but you have a new typo on one CHECK :)<o:p></o:p></p></div><div><p class=MsoNormal>+; CHCEK-NOT: cmp<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>With this typo fixed, LGTM.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>-Quentin<o:p></o:p></p></div><p class=MsoNormal><o:p> </o:p></p><div><div><p class=MsoNormal>On Dec 5, 2013, at 5:19 PM, Weiming Zhao <<a href="mailto:weimingz@codeaurora.org">weimingz@codeaurora.org</a>> wrote:<o:p></o:p></p></div><p class=MsoNormal><br><br><o:p></o:p></p><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Hi Quentin,</span><o:p></o:p></p></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> </span><o:p></o:p></p></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Thanks for review. You’re right. It should give up (return false) when condition code update is needed.</span><o:p></o:p></p></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> </span><o:p></o:p></p></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I also added two test cases for such scenario.</span><o:p></o:p></p></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> </span><o:p></o:p></p></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Thanks,</span><o:p></o:p></p></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Weiming</span><o:p></o:p></p></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> </span><o:p></o:p></p></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation</span><o:p></o:p></p></div></div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> </span><o:p></o:p></p></div><div><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in'><div><p class=MsoNormal><b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>From:</span></b><span class=apple-converted-space><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'> </span></span><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>Quentin Colombet [<a href="mailto:qcolombet@apple.com"><span style='color:purple'>mailto:qcolombet@apple.com</span></a>]<span class=apple-converted-space> </span><br><b>Sent:</b><span class=apple-converted-space> </span>Thursday, December 05, 2013 3:56 PM<br><b>To:</b><span class=apple-converted-space> </span>Weiming Zhao<br><b>Cc:</b><span class=apple-converted-space> </span>llvm-commits<br><b>Subject:</b><span class=apple-converted-space> </span>Re: [Patch][AArch32] Fix vsel in compareinst optimization</span><o:p></o:p></p></div></div></div><div><p class=MsoNormal> <o:p></o:p></p></div><div><p class=MsoNormal>Hi Weiming,<o:p></o:p></p></div><div><div><p class=MsoNormal> <o:p></o:p></p></div></div><div><div><p class=MsoNormal>The following code does not look correct:<o:p></o:p></p></div></div><div><div><p class=MsoNormal>- Sub->getOperand(2).getReg() == SrcReg)<o:p></o:p></p></div></div><div><div><p class=MsoNormal>+ Sub->getOperand(2).getReg() == SrcReg && !IsInstrVSel)<o:p></o:p></p></div></div><div><div><p class=MsoNormal> <o:p></o:p></p></div></div><div><div><p class=MsoNormal>Indeed, if the CMP and SUB have different operands order, the VSEL condition would need to be updated as well if the CMP is removed. Otherwise, we would not select the right operand, would we?<o:p></o:p></p></div></div><div><div><p class=MsoNormal>In other words, I think we are missing an update of the opcode of the VSEL in that case, since no condition code is available here.<o:p></o:p></p></div></div><div><div><p class=MsoNormal>Am I missing something?<o:p></o:p></p></div></div><div><div><div><p class=MsoNormal> <o:p></o:p></p></div></div><div><div><p class=MsoNormal>You have a typo in the tests, this happens twice:<o:p></o:p></p></div></div><div><div><p class=MsoNormal>+; CHCK-NOT: cmp<o:p></o:p></p></div></div><div><div><p class=MsoNormal> <o:p></o:p></p></div></div><div><div><p class=MsoNormal>The E is missing.<o:p></o:p></p></div></div><div><div><p class=MsoNormal> <o:p></o:p></p></div></div><div><div><p class=MsoNormal>Thanks,<o:p></o:p></p></div></div><div><div><div><p class=MsoNormal><span style='font-family:"Helvetica","sans-serif"'>-Quentin</span><o:p></o:p></p></div></div><div><p class=MsoNormal> <o:p></o:p></p></div><div><div><div><p class=MsoNormal>On Dec 5, 2013, at 3:19 PM, Weiming Zhao <<a href="mailto:weimingz@codeaurora.org"><span style='color:purple'>weimingz@codeaurora.org</span></a>> wrote:<o:p></o:p></p></div></div><div><p class=MsoNormal><br><br><br><o:p></o:p></p></div><div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'>Hi,</span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'> </span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'>Attached patch is to fix bug:<span class=apple-converted-space> </span><a href="http://llvm.org/bugs/show_bug.cgi?id=18149"><span style='color:purple'>http://llvm.org/bugs/show_bug.cgi?id=18149</span></a></span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'> </span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'>Basically, the cmp optimization assumes a MI that uses CPSR has an immediate MO for ARMCC. But this is not true for VSEL instructions.</span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'> </span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'>Thanks,</span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'>Weiming</span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'> </span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation</span><o:p></o:p></p></div></div><div><div><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'> </span><o:p></o:p></p></div></div><div><p class=MsoNormal><span style='font-size:9.0pt;font-family:"Helvetica","sans-serif"'><0001-Bug-18149-AArch32-VSel-instructions-has-no-ARMCC-fie.patch>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu"><span style='color:purple'>llvm-commits@cs.uiuc.edu</span></a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits"><span style='color:purple'>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</span></a></span><o:p></o:p></p></div></div></div><div><p class=MsoNormal> <o:p></o:p></p></div></div></div><p class=MsoNormal><span style='font-size:9.0pt;font-family:"Helvetica","sans-serif"'><0001-Bug-18149-AArch32-VSel-instructions-has-no-ARMCC-fie.patch><o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p></div></body></html>