<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Feb 6, 2015, at 11:07 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com" class="">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Feb 5, 2015 at 5:36 PM, Michael Zolotukhin <span dir="ltr" class=""><<a href="mailto:mzolotukhin@apple.com" target="_blank" class="">mzolotukhin@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class="">Hi Chandler,<div class=""><br class=""></div><div class="">We now have a new user of SimplifyBinOp in LoopUnroll, I’ve updated the patch accordingly:</div><div class=""><br class=""></div><div class=""></div></div><br class=""><div style="word-wrap:break-word" class=""><div class=""></div><div class=""><br class=""></div><div class="">Does it look good?</div></div></blockquote><div class=""><br class=""></div><div class="">One issue:</div><div class=""><br class=""></div><div class=""><div class="">+static Value *SimplifyFPBinOp(unsigned Opcode, Value *LHS, Value *RHS,</div><div class="">+ const Query &Q, unsigned MaxRecurse,</div><div class="">+ const FastMathFlags &FMF) {</div><div class="">+ switch (Opcode) {</div><div class="">+ case Instruction::FAdd:</div><div class="">+ return SimplifyFAddInst(LHS, RHS, FMF, Q, MaxRecurse);</div></div><div class=""><br class=""></div><div class="">We shouldn't have different argument ordering between the internal SimplifyFPBinOp and SimplifyFAddInst. Pass FMF after RHS in SimplifyFPBinOp?</div><div class=""><br class=""></div><div class="">LGTM with that change.</div></div></div></div></div></blockquote>Thanks, Chandler! Fixed and committed in r228432. I’ll follow up with a test case soon.</div><div><br class=""></div><div>Michael</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Michael<br class=""><div class=""><blockquote type="cite" class=""><div class="">On Jan 30, 2015, at 12:10 PM, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com" target="_blank" class="">mzolotukhin@apple.com</a>> wrote:</div><br class=""><div class=""><div style="word-wrap:break-word" class="">Hi Chandler,<div class=""><br class=""></div><div class="">Thanks for the feedback, updated patch is attached.</div><div class=""><br class=""></div><div class="">I added SimplifyFPBinOp routine, but decided to keep SimplifyBinOp untouched. I.e. if we use SimplifyBinOp for FP operator, we just use default fast math flags - that’s what we have currently. It doesn’t lead to incorrect code, but it’s conservative, and it’s better to use SimplifyFPBinOp with explicitly specified flags. I’m not sure if that’s what you meant though - are you fine with this approach?</div><div class=""><br class=""></div><div class="">As for clang format - I did use it last time, and used this time as well (I believe the indentation was strange, because clang-format doesn’t always try to fold several short lines that could fit to 80 characters if placed in one line).</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""></div></div><span class=""><inst-simplify-fast-math-3.patch></span><div style="word-wrap:break-word" class=""><div class=""></div><div class=""><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Michael</div></div><div class=""><br class=""></div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Jan 29, 2015, at 6:13 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="">chandlerc@google.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Yea, I think this is the right direction.<div class=""><br class=""></div><div class="">Let's clean up how the API is structured though:</div><div class=""><br class=""></div><div class="">1) pass FastMathFlags by value, and default construct the obvious value.</div><div class=""><br class=""></div><div class="">2) what about making a SimplifyFPBinOp which *requires* a flags parameter in addition to the opcode? Then SimplifyBinOp could delegate to this with a default constructed flags for the FP cases. In the caller code, I feel like this would be a touch cleaner / less prone to minor errors. Because we already have to test whether the operation is FP and could have flags, we might as well call a specialized routine.</div><div class=""><br class=""></div><div class="">As a nit-pick, it'd be nice to use clang-format on the caller, I think it will do a nice job with these long argument lists. (Unless you did, and clang-format is just choosing this... somewhat surprising layout?)</div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Jan 29, 2015 at 6:02 PM, Michael Zolotukhin <span dir="ltr" class=""><<a href="mailto:mzolotukhin@apple.com" target="_blank" class="">mzolotukhin@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jan 29, 2015, at 6:01 PM, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com" target="_blank" class="">mzolotukhin@apple.com</a>> wrote:</div><br class=""><div class=""><div style="word-wrap:break-word" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Jan 29, 2015, at 4:21 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="">chandlerc@google.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Do we use the context instruction to extract fast math flags elsewhere?<div class=""><br class=""></div><div class="">I wouldn't expect that to be reliably at all. Nothing *requires* the context instruction to be the one governing the semantics.</div><div class=""><br class=""></div><div class="">I think we need to pass the fast math flags directly into SimplifyBinOp for this to work.</div></div></div></blockquote><div class=""><br class=""></div><div class="">I tried to avoid adding a new argument, but you are right: using the context instruction isn’t appropriate for this.</div><div class=""><br class=""></div><div class="">Updated patch is attached, is it better?</div></div></div></div></blockquote></span>Oops, I forgot the patch. Here it is:</div><div class=""><br class=""></div><div class=""></div></div><br class=""><div style="word-wrap:break-word" class=""><div class=""></div><div class=""><br class=""></div><div class="">Michael<br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap:break-word" class=""><div class=""><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">Michael</div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Jan 29, 2015 at 4:12 PM, Michael Zolotukhin <span dir="ltr" class=""><<a href="mailto:mzolotukhin@apple.com" target="_blank" class="">mzolotukhin@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br class="">
<br class="">
Currently InlineCost pass fails to use fast-math flags when analyzing profitability of inlining. Here is a small example to demonstrate that:<br class="">
<br class="">
float foo(float *a, float b) {<br class="">
float r;<br class="">
if (a[1] * b)<br class="">
r = a[1]+a[2]/a[3]-a[4]*a[5]+a[6]-a[7]+a[8]*a[9]+<br class="">
a[11]+a[12]/a[13]-a[14]*a[15]+a[16]-a[17]+a[18]*a[19]+<br class="">
a[21]+a[22]/a[23]-a[24]*a[25]+a[26]-a[27]+a[28]*a[29]+<br class="">
a[31]+a[32]/a[33]-a[34]*a[35]+a[36]-a[37]+a[38]*a[39]+<br class="">
a[41]+a[42]/a[43]-a[44]*a[45]+a[46]-a[47]+a[48]*a[49]+<br class="">
a[51]+a[52]/a[53]-a[54]*a[55]+a[56]-a[57]+a[58]*a[59]+<br class="">
a[61]+a[62]/a[63]-a[64]*a[65]+a[66]-a[67]+a[68]*a[69]+<br class="">
a[0]*a[0]+b;<br class="">
else<br class="">
r = 1;<br class="">
return r;<br class="">
}<br class="">
float boo(float *a) {<br class="">
return foo(a, 0.0);<br class="">
}<br class="">
<br class="">
Here inliner can’t figure out that a[1]*b can be folded to 0 if the call is inlined, and decides not to inline.<br class="">
<br class="">
That happens because inliner drops fast-math flags in SimplifyBinOp. The attached patch fixes it by looking into the instruction math flags. Does it look good?<br class="">
<br class="">
Without patch:<br class="">
> clang inline.c -O3 -S -Rpass=inline -ffast-math<br class="">
// Empty output<br class="">
<br class="">
With patch:<br class="">
> clang inline.c -O3 -S -Rpass=inline -ffast-math<br class="">
inline.c:17:10: remark: foo inlined into boo [-Rpass=inline]<br class="">
return foo(a, 0.0);<br class="">
^<br class="">
<br class="">
<br class=""><br class="">
<br class="">
Thanks,<br class="">
Michael<br class="">_______________________________________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="">llvm-commits@cs.uiuc.edu</a><br class="">
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class="">
<br class=""></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></div><br class=""></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div></div>_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="">llvm-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class=""></div></blockquote></div><br class=""></div></div><br class=""></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>