<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 5, 2015 at 5:36 PM, Michael Zolotukhin <span dir="ltr"><<a href="mailto:mzolotukhin@apple.com" target="_blank">mzolotukhin@apple.com</a>></span> wrote:<br><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">Hi Chandler,<div><br></div><div>We now have a new user of SimplifyBinOp in LoopUnroll, I’ve updated the patch accordingly:</div><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div><div><br></div><div>Does it look good?</div></div></blockquote><div><br></div><div>One issue:</div><div><br></div><div><div>+static Value *SimplifyFPBinOp(unsigned Opcode, Value *LHS, Value *RHS,</div><div>+ const Query &Q, unsigned MaxRecurse,</div><div>+ const FastMathFlags &FMF) {</div><div>+ switch (Opcode) {</div><div>+ case Instruction::FAdd:</div><div>+ return SimplifyFAddInst(LHS, RHS, FMF, Q, MaxRecurse);</div></div><div><br></div><div>We shouldn't have different argument ordering between the internal SimplifyFPBinOp and SimplifyFAddInst. Pass FMF after RHS in SimplifyFPBinOp?</div><div><br></div><div>LGTM with that change.</div><div> </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"><div><br></div><div>Thanks,</div><div>Michael<br><div><blockquote type="cite"><div>On Jan 30, 2015, at 12:10 PM, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com" target="_blank">mzolotukhin@apple.com</a>> wrote:</div><br><div><div style="word-wrap:break-word">Hi Chandler,<div><br></div><div>Thanks for the feedback, updated patch is attached.</div><div><br></div><div>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><br></div><div>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><br></div><div><br></div><div></div></div><span><inst-simplify-fast-math-3.patch></span><div style="word-wrap:break-word"><div></div><div><div><br></div><div>Thanks,</div><div>Michael</div></div><div><br></div><div><br><div><blockquote type="cite"><div>On Jan 29, 2015, at 6:13 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div><br><div><div dir="ltr">Yea, I think this is the right direction.<div><br></div><div>Let's clean up how the API is structured though:</div><div><br></div><div>1) pass FastMathFlags by value, and default construct the obvious value.</div><div><br></div><div>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><br></div><div>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><div class="gmail_quote">On Thu, Jan 29, 2015 at 6:02 PM, Michael Zolotukhin <span dir="ltr"><<a href="mailto:mzolotukhin@apple.com" target="_blank">mzolotukhin@apple.com</a>></span> wrote:<br><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"><br><div><span><blockquote type="cite"><div>On Jan 29, 2015, at 6:01 PM, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com" target="_blank">mzolotukhin@apple.com</a>> wrote:</div><br><div><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Jan 29, 2015, at 4:21 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div><br><div><div dir="ltr">Do we use the context instruction to extract fast math flags elsewhere?<div><br></div><div>I wouldn't expect that to be reliably at all. Nothing *requires* the context instruction to be the one governing the semantics.</div><div><br></div><div>I think we need to pass the fast math flags directly into SimplifyBinOp for this to work.</div></div></div></blockquote><div><br></div><div>I tried to avoid adding a new argument, but you are right: using the context instruction isn’t appropriate for this.</div><div><br></div><div>Updated patch is attached, is it better?</div></div></div></div></blockquote></span>Oops, I forgot the patch. Here it is:</div><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div><div><br></div><div>Michael<br><blockquote type="cite"><div><div style="word-wrap:break-word"><div><div><br></div><div><br></div><div><br></div><div>Michael</div><div><br></div><blockquote type="cite"><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 29, 2015 at 4:12 PM, Michael Zolotukhin <span dir="ltr"><<a href="mailto:mzolotukhin@apple.com" target="_blank">mzolotukhin@apple.com</a>></span> wrote:<br><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>
<br>
Currently InlineCost pass fails to use fast-math flags when analyzing profitability of inlining. Here is a small example to demonstrate that:<br>
<br>
float foo(float *a, float b) {<br>
float r;<br>
if (a[1] * b)<br>
r = a[1]+a[2]/a[3]-a[4]*a[5]+a[6]-a[7]+a[8]*a[9]+<br>
a[11]+a[12]/a[13]-a[14]*a[15]+a[16]-a[17]+a[18]*a[19]+<br>
a[21]+a[22]/a[23]-a[24]*a[25]+a[26]-a[27]+a[28]*a[29]+<br>
a[31]+a[32]/a[33]-a[34]*a[35]+a[36]-a[37]+a[38]*a[39]+<br>
a[41]+a[42]/a[43]-a[44]*a[45]+a[46]-a[47]+a[48]*a[49]+<br>
a[51]+a[52]/a[53]-a[54]*a[55]+a[56]-a[57]+a[58]*a[59]+<br>
a[61]+a[62]/a[63]-a[64]*a[65]+a[66]-a[67]+a[68]*a[69]+<br>
a[0]*a[0]+b;<br>
else<br>
r = 1;<br>
return r;<br>
}<br>
float boo(float *a) {<br>
return foo(a, 0.0);<br>
}<br>
<br>
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>
<br>
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>
<br>
Without patch:<br>
> clang inline.c -O3 -S -Rpass=inline -ffast-math<br>
// Empty output<br>
<br>
With patch:<br>
> clang inline.c -O3 -S -Rpass=inline -ffast-math<br>
inline.c:17:10: remark: foo inlined into boo [-Rpass=inline]<br>
return foo(a, 0.0);<br>
^<br>
<br>
<br><br>
<br>
Thanks,<br>
Michael<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></blockquote></div><br></div>
</div></blockquote></div><br></div></div></blockquote></div><br></div><br></blockquote></div><br></div>
</div></blockquote></div><br></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></div></blockquote></div><br></div></div><br></blockquote></div><br></div></div>