<div dir="ltr">Hi Sergey,<div><br></div><div>Did you forget to attach your new patch?</div><div><br></div><div>I tried the spec benchmarks by disabling that code of inverting the condition, and only see the following performance changes and no change for all others.</div>
<div><br></div><div>164.gzip (ref) +1.76%</div><div>458.sjeng (train) + 2.19%</div><div>471.omnetpp (train) -1.43%</div><div>473.astar (train) -1.51%</div><div><br></div><div>Hopefully we can understand why this could happen, but maybe this is just a heuristic result depending on the real control flow and workload.</div>
<div><br></div><div>Thanks,<br></div><div>-Jiangning</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-24 16:17 GMT+08:00 Sergey Dmitrouk <span dir="ltr"><<a href="mailto:sdmitrouk@accesssoftek.com" target="_blank">sdmitrouk@accesssoftek.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello Jiangning,<br>
<br>
Thanks for your comments.<br>
<br>
> 1) I expect the fix should be insideA getAArch64Cmp.<br>
>...<br>
<div class="">> but essentially getAArch64Cmp missed the case of (x < 1) -> (x <= 0).<br>
<br>
</div>It was initial placement of the fix, but the function doesn't seem to<br>
perform transformations like that. It updates conditions only when<br>
immediate values are not legal. There is no comment for the function,<br>
so I'm not sure whether such checks fit there, but I moved the change.<br>
<div class=""><br>
> 2) Your comment is inconsistent with your code.<br>
<br>
</div>Thanks, it's probably because of inverted conditions in DAGs.<br>
<div class=""><br>
> So now I'm wondering how to justify this is always meaningful for AArch64?<br>
<br>
</div>I wasn't sure whether it's worth such change, but as an option something<br>
like TargetLowering::isInversionBeneficial(SDValue Cond) can be added,<br>
but I don't know whether it's possible to check for conditions like<br>
"(a < 0 && b == c || a > 0 && b == d)" to do not block inversion for all<br>
cases.<br>
<br>
Attached updated patch at least to see whether the fix fits well in<br>
getAArch64Cmp().<br>
<br>
Regards,<br>
Sergey<br>
<div class=""><br>
On Wed, Jul 23, 2014 at 10:05:37PM -0700, Jiangning Liu wrote:<br>
> Hi Sergey,<br>
</div>> 1) I expect the fix should be insideA getAArch64Cmp.<br>
<div class="">> 2) Your comment is inconsistent with your code. Your code is to transform<br>
> (x < 1) to be (x<=0), rather than "Turn "x > 1" condition into "x >= 0"".<br>
> I also noticed we have the following transformation for if condition (x <<br>
> 0) in back-end,<br>
> stage 1: (x < 0) -> (x >= 0), i.e. (x<0) and invert the targets.<br>
> stage 2: (x >= 0) -> (x > -1). This happens in combine1.<br>
> stage 3: (x > -1) -> (x >= 0) in getAArch64Cmp.<br>
> For if condition (x > 0), the transformation is similar. Your patch is<br>
> trying to cover this case, but essentially getAArch64Cmp missed the case<br>
> of (x < 1) -> (x <= 0).<br>
> However, as you can see the root cause of generating the comparison with<br>
> constant 1 is stage 1. This happens<br>
</div>> insideA SelectionDAGBuilder::visitSwitchCase<br>
> A // If the lhs block is the next block, invert the condition so that we<br>
> can<br>
> A // fall through to the lhs instead of the rhs block.<br>
> A if (CB.TrueBB == NextBlock) {<br>
> A A std::swap(CB.TrueBB, CB.FalseBB);<br>
> A A SDValue True = DAG.getConstant(1, Cond.getValueType());<br>
> A A Cond = DAG.getNode(ISD::XOR, dl, Cond.getValueType(), Cond, True);<br>
> A }<br>
<div class="">> So now I'm wondering how to justify this is always meaningful for AArch64?<br>
> Thanks,<br>
> -Jiangning<br>
><br>
> 2014-07-23 23:54 GMT+08:00 Sergey Dmitrouk <<a href="mailto:sdmitrouk@accesssoftek.com">sdmitrouk@accesssoftek.com</a>>:<br>
><br>
> Hi,<br>
><br>
> Basing on the following information from [this post][0] by James Molloy:<br>
><br>
</div>> A 2. "if (a < 0 && b == c || a > 0 && b == d)" - the first comparison<br>
> of<br>
> A 'a' against zero is done twice, when the flag results of the first<br>
> A comparison could be used for the second comparison.<br>
><br>
> I've made a patch (attached) that removes this extra comparison. A More<br>
<div class="">> complex cases like comparisons with non-zero immediate values or with<br>
</div>> registers doesn't seem to be task for a code generator. A Comparing with<br>
<div class="">> zero is quite common, so I seems to be worth adding.<br>
><br>
</div>> Please review the patch. A Couldn't find a better place to make the<br>
<div class="HOEnZb"><div class="h5">> change, but I'll be happy to adjust the patch if anyone has better<br>
> ideas.<br>
><br>
> Best regards,<br>
> Sergey<br>
><br>
> 0: <a href="http://article.gmane.org/gmane.comp.compilers.llvm.devel/74269" target="_blank">http://article.gmane.org/gmane.comp.compilers.llvm.devel/74269</a><br>
><br>
> _______________________________________________<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" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>