<html><head></head><body bgcolor="#FFFFFF"><div>Thanks. I think the patch is correct now. But I would very much appreciate another pair of eyes on it. </div><div><br></div><div>Evan<br><br>On Apr 17, 2012, at 11:29 AM, Yin Ma <<a href="mailto:yinma@codeaurora.org">yinma@codeaurora.org</a>> wrote:<br><br></div><div></div><blockquote type="cite"><div><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"><meta name="Generator" content="Microsoft Word 14 (filtered medium)"><base href="x-msg://2179/"><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]--><div class="WordSection1"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">HI Evan,<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"> I have updated the source based on your latest comments.<o:p></o:p></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thank you for reviewing. <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"> Yin <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><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""> Evan Cheng [mailto:evan.cheng@apple.com] <br><b>Sent:</b> Saturday, April 14, 2012 11:51 AM<br><b>To:</b> Yin Ma<br><b>Cc:</b> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b> Re: [llvm-commits] LLVM patch to implement UMLAL/SMLAL Instructions for ARM Architecture.<o:p></o:p></span></p></div></div><p class="MsoNormal"><o:p> </o:p></p><p class="MsoNormal">Sorry, I took a closer look and I find some issues.<o:p></o:p></p><div><p class="MsoNormal"><o:p> </o:p></p></div><div><div><p class="MsoNormal"> // Multiply + accumulate <o:p></o:p></p></div><div><p class="MsoNormal">-def SMLAL : AsMul1I64<0b0000111, (outs GPR:$RdLo, GPR:$RdHi), <o:p></o:p></p></div><div><p class="MsoNormal">- (ins GPR:$Rn, GPR:$Rm), IIC_iMAC64, <o:p></o:p></p></div><div><p class="MsoNormal">+def SMLAL : AsMla1I64<0b0000111, (outs GPR:$RdLo, GPR:$RdHi), <o:p></o:p></p></div><div><p class="MsoNormal">+ (ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi), IIC_iMAC64, <o:p></o:p></p></div><div><p class="MsoNormal"> "smlal", "\t$RdLo, $RdHi, $Rn, $Rm", []>, <o:p></o:p></p></div><div><p class="MsoNormal">- Requires<[IsARM, HasV6]>; <o:p></o:p></p></div><div><p class="MsoNormal">-def UMLAL : AsMul1I64<0b0000101, (outs GPR:$RdLo, GPR:$RdHi), <o:p></o:p></p></div><div><p class="MsoNormal">- (ins GPR:$Rn, GPR:$Rm), IIC_iMAC64, <o:p></o:p></p></div><div><p class="MsoNormal">+ RegConstraint<"$RLo = $RdLo, $RHi = $RdHi">, Requires<[IsARM, HasV6]>; <o:p></o:p></p></div><div><p class="MsoNormal">+def UMLAL : AsMla1I64<0b0000101, (outs GPR:$RdLo, GPR:$RdHi), <o:p></o:p></p></div><div><p class="MsoNormal">+ (ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi), IIC_iMAC64, <o:p></o:p></p></div><div><p class="MsoNormal"> "umlal", "\t$RdLo, $RdHi, $Rn, $Rm", []>, <o:p></o:p></p></div><div><p class="MsoNormal">- Requires<[IsARM, HasV6]>; <o:p></o:p></p></div><div><p class="MsoNormal">+ RegConstraint<"$RLo = $RdLo, $RHi = $RdHi">, Requires<[IsARM, HasV6]>;<o:p></o:p></p></div><div><p class="MsoNormal"><o:p> </o:p></p></div><div><div><p class="MsoNormal">-let Constraints = "@earlyclobber $RdLo,@earlyclobber $RdHi" in { <o:p></o:p></p></div><div><p class="MsoNormal">+let Constraints = "$RLo = $RdLo,$RHi = $RdHi" in { <o:p></o:p></p></div><div><p class="MsoNormal"> def SMLALv5 : ARMPseudoExpand<(outs GPR:$RdLo, GPR:$RdHi), <o:p></o:p></p></div><div><p class="MsoNormal">- (ins GPR:$Rn, GPR:$Rm, pred:$p, cc_out:$s), <o:p></o:p></p></div><div><p class="MsoNormal">+ (ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s), <o:p></o:p></p></div><div><p class="MsoNormal"> 4, IIC_iMAC64, [], <o:p></o:p></p></div><div><p class="MsoNormal">- (SMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, pred:$p, cc_out:$s)>, <o:p></o:p></p></div><div><p class="MsoNormal">+ (SMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s)>, <o:p></o:p></p></div><div><p class="MsoNormal"> Requires<[IsARM, NoV6]>; <o:p></o:p></p></div><div><p class="MsoNormal"> def UMLALv5 : ARMPseudoExpand<(outs GPR:$RdLo, GPR:$RdHi), <o:p></o:p></p></div><div><p class="MsoNormal">- (ins GPR:$Rn, GPR:$Rm, pred:$p, cc_out:$s), <o:p></o:p></p></div><div><p class="MsoNormal">- 4, IIC_iMAC64, [], <o:p></o:p></p></div><div><p class="MsoNormal">- (UMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, pred:$p, cc_out:$s)>, <o:p></o:p></p></div><div><p class="MsoNormal">- Requires<[IsARM, NoV6]>; <o:p></o:p></p></div><div><p class="MsoNormal">+ (ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s), <o:p></o:p></p></div><div><p class="MsoNormal">+ 4, IIC_iMAC64, [], <o:p></o:p></p></div><div><p class="MsoNormal">+ (UMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s)>, <o:p></o:p></p></div><div><p class="MsoNormal">+ Requires<[IsARM, NoV6]>; <o:p></o:p></p></div><div><p class="MsoNormal">+} <o:p></o:p></p></div><div><p class="MsoNormal">+ <o:p></o:p></p></div></div><div><p class="MsoNormal"><o:p> </o:p></p></div><div><p class="MsoNormal">These are beyond 80 columns.<o:p></o:p></p></div><div><p class="MsoNormal"><o:p> </o:p></p></div><div><div><p class="MsoNormal">+ // follow the glue value to get the second add <o:p></o:p></p></div><div><p class="MsoNormal">+ // don't know how much uses of the first add <o:p></o:p></p></div><div><p class="MsoNormal">+ // use while check<o:p></o:p></p></div></div><div><p class="MsoNormal"><o:p> </o:p></p></div><div><p class="MsoNormal">This comment doesn't make sense. Please correct.<o:p></o:p></p></div><div><p class="MsoNormal"><o:p> </o:p></p></div><div><p class="MsoNormal">Evan<o:p></o:p></p></div><div><p class="MsoNormal"><o:p> </o:p></p></div><div><div><p class="MsoNormal">On Apr 12, 2012, at 11:15 AM, Evan Cheng wrote:<o:p></o:p></p></div><p class="MsoNormal"><br><br><o:p></o:p></p><div><p class="MsoNormal">LGTM. Can someone commit this for you?<o:p></o:p></p><div><p class="MsoNormal"><o:p> </o:p></p></div><div><p class="MsoNormal">Evan<o:p></o:p></p></div><div><p class="MsoNormal"><o:p> </o:p></p><div><div><p class="MsoNormal">On Apr 2, 2012, at 10:22 AM, Yin Ma 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,</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 have updated the code based on your comments. I have put more comments</span><o:p></o:p></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">on the code, especially for some conditional statements I cannot remove. Please</span><o:p></o:p></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">review again.</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">Llmlal.diff is the code diff for the updated version</span><o:p></o:p></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Reports.simple.txt and result.txt are the new test results</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"> </span><o:p></o:p></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> Yin</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"> </span><o:p></o:p></p></div><div><div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in;border-width:initial;border-color:initial"><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"">Evan Cheng <a href="mailto:[mailto:evan.cheng@apple.com]">[mailto:evan.cheng@apple.com]</a><span class="apple-converted-space"> </span><br><b>Sent:</b><span class="apple-converted-space"> </span>Thursday, March 15, 2012 11:07 AM<br><b>To:</b><span class="apple-converted-space"> </span>Yin Ma<br><b>Cc:</b><span class="apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b><span class="apple-converted-space"> </span>Re: [llvm-commits] LLVM patch to implement UMLAL/SMLAL Instructions for ARM Architecture.</span><o:p></o:p></p></div></div></div><div><p class="MsoNormal"> <o:p></o:p></p></div><div><p class="MsoNormal"> <o:p></o:p></p></div><div><div><div><p class="MsoNormal">On Mar 15, 2012, at 10:17 AM, Yin Ma 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";color:#1F497D">Hi Evan,</span><o:p></o:p></p></div></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><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> Thank you for review this patch. I will rework on the code style based</span><o:p></o:p></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">on your advice. Then please review it again.</span><o:p></o:p></p></div></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><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">For the question #8</span><o:p></o:p></p></div></div><div><div><p class="MsoNormal">8.<o:p></o:p></p></div></div><div><div><p class="MsoNormal">There are a lot of failures in report.simple.txt. Why is that?<o:p></o:p></p></div></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><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Those failures are not caused by my change. It is due to our current test machine setup.</span><o:p></o:p></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">This patch didn’t increase any number of failures. After reworking on the style, I will</span><o:p></o:p></p></div></div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Look for a better machine to run unit test. The failure number will change.</span><o:p></o:p></p></div></div></div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div><div><p class="MsoNormal">Ok. Please try to get a clean run. Otherwise it's not particularly useful to include as part of the patch review.<o:p></o:p></p></div></div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div><div><div><p class="MsoNormal">Evan<o:p></o:p></p></div></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";color:#1F497D"> </span><o:p></o:p></p></div></div><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><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><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> Yin</span><o:p></o:p></p></div></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><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><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><div style="border:none;border-top:solid windowtext 3.0pt;padding:3.0pt 0in 0in 0in;border-width:initial;border-color:initial;border-width:initial;border-color:initial"><div><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"">Evan Cheng<span class="apple-converted-space"> </span><a href="mailto:[mailto:evan.cheng@apple.com]">[mailto:evan.cheng@apple.com]</a><span class="apple-converted-space"> </span><br><b>Sent:</b><span class="apple-converted-space"> </span>Thursday, March 15, 2012 12:01 AM<br><b>To:</b><span class="apple-converted-space"> </span>Yin Ma<br><b>Cc:</b><span class="apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b><span class="apple-converted-space"> </span>Re: [llvm-commits] LLVM patch to implement UMLAL/SMLAL Instructions for ARM Architecture.</span><o:p></o:p></p></div></div></div></div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div><div><div><p class="MsoNormal">Thanks. Preliminary reviews below.<o:p></o:p></p></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">The first thing I noticed is some of your stylistic choices are different from the rest of the code:<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">if( nValue != 2 ) return SDValue();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">LLVM doesn't use camel case for variable name. Also, there should be a space before '(', not after.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+ }else{<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">Should be } else {<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">Now onto the rest of the patch.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">1.<o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+ case ARMISD::UMLAL:{<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ if (Subtarget->isThumb1Only())<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ break;<o:p></o:p></p></div></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">The check should not be needed, right? ARMISD::UMLAL cannot be formed for Thumb1.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+ // For UMLAL/SMLAL<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ setTargetDAGCombine(ISD::ADDC);<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ setTargetDAGCombine(ISD::ADDE);<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+<o:p></o:p></p></div></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">Should these be guarded with !(Subtarget->isThumb1Only()?<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">2.<o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+ case ISD::ADDE: return PerformADDECCombine(N, DCI, Subtarget);<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ case ISD::ADDC: return PerformADDECCombine(N, DCI, Subtarget);<o:p></o:p></p></div></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">That's silly. It should be:<o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+ case ISD::ADDE:<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ case ISD::ADDC: return PerformADDECCombine(N, DCI, Subtarget);<o:p></o:p></p></div></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">3. Can you add more comments to AddCombineTo64bitMLAL()? Please describe what the routine is trying to match.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">4.<o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+ // The second use must be a glue to a add<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ int nValue = N->getNumValues();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ if( nValue != 2 ) return SDValue();<o:p></o:p></p></div></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">if (N->getNumValues() != 2) return SDValue();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">The check isn't necessary. ADDE always produce two values.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">5.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+ EVT GVT = N->getValueType(1);<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ if( VT != MVT::i32 || GVT != MVT::Glue ) return SDValue();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">VT must be MVT::i32 after legalization, right? Are there cases where GVT might not be MVT::Glue?<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ // look for glue value<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ SDNode::use_iterator UI = N->use_begin();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ SDNode::use_iterator UE = N->use_end();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ SDNode* HiAdd = NULL;<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ while( UI != UE ){<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ SDUse& Nuse = UI.getUse();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ if( Nuse.getResNo() == 1 ){<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ HiAdd = Nuse.getUser();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ break;<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ }<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ UI++;<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ }<o:p></o:p></p></div></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">Is a loop necessary? A glue value can only have a single use.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">6. The rest of the routine has more stylistic issues. Also it's hard to understand without high level description.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">7. <o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+/// PerformADDECCombine - Target-specific dag combine xforms for ISD::ADDE & ISD::ADDC for UMLAL.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+///<o:p></o:p></p></div></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">Please be aware of 80 col violation.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><div><p class="MsoNormal">+ SDValue Result = AddCombineTo64bitMLAL(N, DCI, Subtarget);<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ if (Result.getNode())<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ return Result;<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ // If that didn't work, try again with the operands commuted.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+ return SDValue();<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">+}<o:p></o:p></p></div></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">Isn't this just?<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">return AddCombineTo64bitMLAL(N, DCI, Subtarget);<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">This routine seems unnecessary.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">The comment indicates it should try again with operands commuted. That should be done in AddCombineTo64bitMLAL, no?<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">8.<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal">There are a lot of failures in report.simple.txt. Why is that?<o:p></o:p></p></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div><div><div><div><p class="MsoNormal">Evan<o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div><div><div><div><div><p class="MsoNormal">On Mar 8, 2012, at 4:15 PM, Yin Ma <<a href="mailto:yinma@codeaurora.org">yinma@codeaurora.org</a>> wrote:<o:p></o:p></p></div></div></div><div><div><p class="MsoNormal"><br><br><br><br><o:p></o:p></p></div></div><div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">The current definition of UMLAL/SMLAL in LLVM for ARM is not used and the</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">definition is not correct because the instruction reads the four values</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">as the input values instead of two values defined in the .td file.</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">I have created a bugzilla entry regarding to this issue:</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"><a href="http://llvm.org/bugs/show_bug.cgi?id=12213"><span style="color:purple">http://llvm.org/bugs/show_bug.cgi?id=12213</span></a></span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">I am proposing a patch not only fixed the definition but also added the corresponding</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">generation algorithm on DAG. This algorithm only operates on ARM backend. It identifies the</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">opportunity of conversions during DAG process.</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">llmla.diff is the code change</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">longMAC.ll is the test case for ARM</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">longMACt.ll is the test case for Thumb2</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">report.simple.txt is the result from test-suites</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">result.txt is the result from test</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">Please give a review. Thanks,</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> Yin</span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p></div></div></div><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p></div></div></div><div><div><p class="MsoNormal"><span style="font-size:13.5pt;font-family:"Helvetica","sans-serif";color:black"><llmlal.diff><longMAC.ll><longMACt.ll><report.simple.txt><result.txt>_______________________________________________<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><div><div><p class="MsoNormal"> <o:p></o:p></p></div></div></div></div></div></div></div></div><div><p class="MsoNormal"> <o:p></o:p></p></div><p class="MsoNormal"><llmlal.diff><longMAC.ll><longMACt.ll><report.simple.txt><result.txt><o:p></o:p></p></div></div><p class="MsoNormal"><o:p> </o:p></p></div></div><p class="MsoNormal">_______________________________________________<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">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><o:p></o:p></p></div><p class="MsoNormal"><o:p> </o:p></p></div></div></div></blockquote><blockquote type="cite"><div><llmlal.diff></div></blockquote><blockquote type="cite"><div><longMAC.ll></div></blockquote><blockquote type="cite"><div><longMACt.ll></div></blockquote><blockquote type="cite"><div><report.simple.txt></div></blockquote><blockquote type="cite"><div><result.txt></div></blockquote></body></html>