<span style="background-color:rgb(255,255,255)"><font color="#222222" face="arial, sans-serif">Thanks for review. Committed revision 161230.</font></span> <br><br>Jush<br><br><div class="gmail_quote">On Thu, Aug 2, 2012 at 10:41 PM, Chad Rosier <span dir="ltr"><<a href="mailto:mcrosier@apple.com" target="_blank">mcrosier@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF"><div>Please apply with those changes.</div><div><br></div><div> Chad<br><br><br></div><div class="im">
<div><br>On Aug 2, 2012, at 1:33 AM, Jush Lu <<a href="mailto:jush.msn@gmail.com" target="_blank">jush.msn@gmail.com</a>> wrote:<br><br></div><div></div></div><div><div class="h5"><blockquote type="cite"><div><br><b style="font-family:'LiHei Pro';font-size:medium;font-weight:normal"><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">Hi,</span><br>
<span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap"></span><br><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">Attachment is new patch, basically I use the test cases you gave in last email, I just add few lines for FileCheck.</span><br>
<span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap"></span><br><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">I think falling back to selection DAG isel is really better way when the shift amount is zero or greater than the width of the value type, so I modified code for this.</span><br>
<span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap"></span><br><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">I also modified code as Jim’ suggestion. </span><br>
<span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap"></span><br><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">Thanks,</span><br><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">Jush </span></b><div>
<br></div><div><br></div><div><div class="gmail_quote">On Thu, Aug 2, 2012 at 2:57 AM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div>On Aug 1, 2012, at 11:18 AM, Chad Rosier <<a href="mailto:mcrosier@apple.com" target="_blank">mcrosier@apple.com</a>> wrote:</div><br><blockquote type="cite">
<div style="word-wrap:break-word">Jush,<div>In the case that the shift amount is zero or greater then the width of the value type, should we just fall back to selection DAG isel to ensure we have consistent behavior (in the event that selection DAG isel changes)? Perhaps someone else could comment on this.</div>
</div></blockquote><div><br></div><div><br></div></div>This seems reasonable to me.</div><div><br></div><div>Additional minor thing:</div><div><br></div><div><div>+ if (isa<ConstantInt>(Src2Value)) {</div><div>+ Opc = ARM::MOVsi;</div>
<div>+ const ConstantInt *CI = cast<ConstantInt>(Src2Value);</div></div><div><br></div><div>This can be simplified to:</div><div>if (const ConstantInt *CI = dyn_cast<ConstantInt>(Src2Value)) {</div><div> Opc = ARM::MOVsi;</div>
<div><br></div><div><br></div><div>-Jim</div><div><br><blockquote type="cite"><div><div><div style="word-wrap:break-word"><div><div><br></div><div>My only other suggestion would be to simplify the test cases. For example,</div>
<div><br></div><div><div>define i32 @shl() nounwind ssp {</div><div>entry:</div><div> %shl = shl i32 -1, 2</div><div> ret i32 %shl</div><div>}</div><div><br></div><div>define i32 @shl_reg(i32 %src1, i32 %src2) nounwind ssp {</div>
<div>entry:</div><div> %shl = shl i32 %src1, %src2</div><div> ret i32 %shl</div><div>}</div><div><br></div><div>define i32 @lshr() nounwind ssp {</div><div>entry:</div><div> %lshr = lshr i32 -1, 2</div><div> ret i32 %lshr</div>
<div>}</div><div><br></div><div>define i32 @lshr_reg(i32 %src1, i32 %src2) nounwind ssp {</div><div>entry:</div><div> %lshr = lshr i32 %src1, %src2</div><div> ret i32 %lshr</div><div>}</div><div><br></div><div>define i32 @ashr() nounwind ssp {</div>
<div>entry:</div><div> %ashr = ashr i32 -1, 2</div><div> ret i32 %ashr</div><div>}</div><div><br></div><div>define i32 @ashr_reg(i32 %src1, i32 %src2) nounwind ssp {</div><div>entry:</div><div> %ashr = ashr i32 %src1, %src2</div>
<div> ret i32 %ashr</div><div>}</div></div><div><br></div><div><div>Basically, I ran mem2reg on your test cases… :) </div><div><br></div><div>While I realize this isn't the type of code fast-isel is going to generate, this is much more concise in terms of what you're trying to test (and that's all we care about).</div>
<div><br></div><div>Please apply after updating the test cases. If anyone disagrees with the handing of the zero/32 shift we can handle that as a post commit; I'm ok with leaving as is..</div><div><br></div><div> Chad</div>
<div><br></div><div><br><div><div>On Jul 31, 2012, at 11:05 PM, Jush Lu wrote:</div><br><blockquote type="cite"><b style="font-family:'LiHei Pro';font-size:medium;font-weight:normal"><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">Hi</span><br>
<span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap"></span><br><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">This patch makes ARMFastISel able to handle shl, lshr, and ashr instructions, please help me review it, thanks.</span><br>
<span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap"></span><br><span style="font-size:15px;font-family:Arial;vertical-align:baseline;white-space:pre-wrap">Jush</span></b>
<span><fast-isel-shifter.patch></span></blockquote></div><br></div></div></div></div></div></div>_______________________________________________<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></blockquote></div><br></div></blockquote></div><br></div>
</div></blockquote></div></div><blockquote type="cite"><div><fast-isel-shifter.patch></div></blockquote></div></blockquote></div><br>