<div dir="ltr">Hi Sergey,<div><br></div><div>Would it be more clear on logic if you move that piece of code to be inside the else branch of "if (!isLegalArithImmed(C)) { ... } else {...}"?</div><div><br></div><div>
I think it would be clear that (C==1) is legal, but we still want to optimize it, and any more optimization cases falling into "legal" scenario would be easily added in future.</div><div><br></div><div>I will be OK if you can simply make this change.</div>
<div><br></div><div>Thanks,</div><div>-Jiangning</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-25 15:21 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">Hi Jiangning,<br>
<div class=""><br>
> Did you forget to attach your new patch?<br>
<br>
</div>Yes, sorry for that. It's attached now.<br>
<div class=""><br>
> Hopefully we can understand why this could happen, but maybe this is just<br>
> a heuristic result depending on the real control flow and workload.<br>
<br>
</div>As that code is common for all supported architectures, maybe it is<br>
important for only some of them and doesn't cause significant performance<br>
changes for others?<br>
<br>
Best regards,<br>
Sergey<br>
<div class=""><br>
On Thu, Jul 24, 2014 at 07:05:43PM -0700, Jiangning Liu wrote:<br>
> Hi Sergey,<br>
</div><div class="">> Did you forget to attach your new patch?<br>
> I tried the spec benchmarks by disabling that code of inverting the<br>
> condition, and only see the following performance changes and no change<br>
> for all others.<br>
</div>> 164.gzip (ref) A +1.76%<br>
<div class="">> 458.sjeng (train) + 2.19%<br>
> 471.omnetpp (train) -1.43%<br>
> 473.astar (train) -1.51%<br>
> Hopefully we can understand why this could happen, but maybe this is just<br>
> a heuristic result depending on the real control flow and workload.<br>
> Thanks,<br>
> -Jiangning<br>
><br>
> 2014-07-24 16:17 GMT+08:00 Sergey Dmitrouk <<a href="mailto:sdmitrouk@accesssoftek.com">sdmitrouk@accesssoftek.com</a>>:<br>
><br>
> Hello Jiangning,<br>
><br>
> Thanks for your comments.<br>
><br>
> > 1) I expect the fix should be insideA getAArch64Cmp.<br>
> >...<br>
> > but essentially getAArch64Cmp missed the case of (x < 1) -> (x <= 0).<br>
><br>
> It was initial placement of the fix, but the function doesn't seem to<br>
</div>> perform transformations like that. A It updates conditions only when<br>
> immediate values are not legal. A There is no comment for the function,<br>
<div class="">> so I'm not sure whether such checks fit there, but I moved the change.<br>
> > 2) Your comment is inconsistent with your code.<br>
><br>
> Thanks, it's probably because of inverted conditions in DAGs.<br>
> > So now I'm wondering how to justify this is always meaningful for<br>
> AArch64?<br>
><br>
> 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>
> On Wed, Jul 23, 2014 at 10:05:37PM -0700, Jiangning Liu wrote:<br>
</div>> > A A Hi Sergey,<br>
> > A A 1) I expect the fix should be insideA getAArch64Cmp.<br>
> > A A 2) Your comment is inconsistent with your code. Your code is to<br>
> transform<br>
> > A A (x < 1) to be (x<=0), rather than "Turn "x > 1" condition into "x<br>
> >= 0"".<br>
> > A A I also noticed we have the following transformation for if<br>
> condition (x <<br>
> > A A 0) in back-end,<br>
> > A A stage 1: (x < 0) -> (x >= 0), i.e. (x<0) and invert the targets.<br>
> > A A stage 2: (x >= 0) -> (x > -1). This happens in combine1.<br>
> > A A stage 3: (x > -1) -> (x >= 0) in getAArch64Cmp.<br>
> > A A For if condition (x > 0), the transformation is similar. Your<br>
> patch is<br>
> > A A trying to cover this case, but essentially getAArch64Cmp missed<br>
> the case<br>
> > A A of (x < 1) -> (x <= 0).<br>
> > A A However, as you can see the root cause of generating the<br>
> comparison with<br>
> > A A constant 1 is stage 1. This happens<br>
> > A A insideA SelectionDAGBuilder::visitSwitchCase<br>
> > A A A A // If the lhs block is the next block, invert the condition<br>
> so that we<br>
> > A A can<br>
> > A A A A // fall through to the lhs instead of the rhs block.<br>
> > A A A A if (CB.TrueBB == NextBlock) {<br>
> > A A A A A A std::swap(CB.TrueBB, CB.FalseBB);<br>
> > A A A A A A SDValue True = DAG.getConstant(1, Cond.getValueType());<br>
> > A A A A A A Cond = DAG.getNode(ISD::XOR, dl, Cond.getValueType(),<br>
> Cond, True);<br>
> > A A A A }<br>
> > A A So now I'm wondering how to justify this is always meaningful for<br>
> AArch64?<br>
> > A A Thanks,<br>
> > A A -Jiangning<br>
> ><br>
> > A A 2014-07-23 23:54 GMT+08:00 Sergey Dmitrouk<br>
> <<a href="mailto:sdmitrouk@accesssoftek.com">sdmitrouk@accesssoftek.com</a>>:<br>
> ><br>
> > A A A Hi,<br>
> ><br>
> > A A A Basing on the following information from [this post][0] by<br>
> James Molloy:<br>
> ><br>
> > A A A A A 2. "if (a < 0 && b == c || a > 0 && b == d)" - the first<br>
> comparison<br>
> > A A A of<br>
> > A A A A A 'a' against zero is done twice, when the flag results of<br>
> the first<br>
> > A A A A A comparison could be used for the second comparison.<br>
> ><br>
> > A A A I've made a patch (attached) that removes this extra<br>
> comparison. A More<br>
> > A A A complex cases like comparisons with non-zero immediate values<br>
> or with<br>
> > A A A registers doesn't seem to be task for a code generator. A<br>
> Comparing with<br>
> > A A A zero is quite common, so I seems to be worth adding.<br>
> ><br>
> > A A A Please review the patch. A Couldn't find a better place to<br>
> make the<br>
> > A A A change, but I'll be happy to adjust the patch if anyone has<br>
> better<br>
> > A A A ideas.<br>
> ><br>
> > A A A Best regards,<br>
> > A A A Sergey<br>
> ><br>
> > A A A 0:<br>
> <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>
> > A A A _______________________________________________<br>
> > A A A llvm-commits mailing list<br>
> > A A A <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > A A A <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>