<div dir="ltr">Hi Sergey,<div><br></div><div>I found your patch would trigger two failures for llvm regression test,</div><div><br></div><div>test/CodeGen/AArch64/arm64-ccmp.ll<br></div><div>test/CodeGen/AArch64/arm64-cse.ll<br>
</div><div><br></div><div>Have you tried that?</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-29 10:02 GMT+08:00 Jiangning Liu <span dir="ltr"><<a href="mailto:liujiangning1@gmail.com" target="_blank">liujiangning1@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Sergey,<div><br></div><div>I just noticed your test case is checking the "comment" in assembly code. I think it would be better if you can simply check sequential b.xx instructions like,</div>
<div>
<br></div><div>; CHECK: cmp</div><div>; CHECK: <a href="http://b.lt" target="_blank">b.lt</a></div><div>; CHECK-NOT: cmp</div><div>; CHECK: b.le</div><div><br></div><div>This way the test can be more stable, because the internal symbol like %lor.lhs.false can be easily changed from time to time.</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-28 16:40 GMT+08:00 Sergey Dmitrouk <span dir="ltr"><<a href="mailto:sdmitrouk@accesssoftek.com" target="_blank">sdmitrouk@accesssoftek.com</a>></span>:<div>
<div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Jiangning,<br>
<br>
You're right, I just thought it might make sense to leave a chance of<br>
catching illegal immediate values after optimizations, which is actually<br>
not something to consider in this case with one and zero.<br>
<br>
Also, maybe<br>
<br>
C = (RHS.getValueType() == MVT::i32) ? (uint32_t)(C - 1) : (C - 1);<br>
<br>
worth changing to<br>
<br>
C = 0ULL;<br>
<br>
It's left from my tries to generalize optimization, but it might be<br>
useful if someone will add additional condition.<br>
<br>
Thanks,<br>
Sergey<br>
<div><br>
On Sun, Jul 27, 2014 at 10:30:45PM -0700, Jiangning Liu wrote:<br>
> Hi Sergey,<br>
</div><div>> Would it be more clear on logic if you move that piece of code to be<br>
> inside the else branch of "if (!isLegalArithImmed(C)) { ... } else {...}"?<br>
> I think it would be clear that (C==1) is legal, but we still want to<br>
> optimize it, and any more optimization cases falling into "legal" scenario<br>
> would be easily added in future.<br>
> I will be OK if you can simply make this change.<br>
> Thanks,<br>
> -Jiangning<br>
><br>
> 2014-07-25 15:21 GMT+08:00 Sergey Dmitrouk <<a href="mailto:sdmitrouk@accesssoftek.com" target="_blank">sdmitrouk@accesssoftek.com</a>>:<br>
><br>
> Hi Jiangning,<br>
> > Did you forget to attach your new patch?<br>
><br>
</div>> Yes, sorry for that. A It's attached now.<br>
<div>> > Hopefully we can understand why this could happen, but maybe this is<br>
> just<br>
> > a heuristic result depending on the real control flow and workload.<br>
><br>
> As that code is common for all supported architectures, maybe it is<br>
> important for only some of them and doesn't cause significant<br>
> performance<br>
> changes for others?<br>
><br>
> Best regards,<br>
> Sergey<br>
> On Thu, Jul 24, 2014 at 07:05:43PM -0700, Jiangning Liu wrote:<br>
</div>> > A A Hi Sergey,<br>
> > A A Did you forget to attach your new patch?<br>
> > A A I tried the spec benchmarks by disabling that code of inverting<br>
> the<br>
> > A A condition, and only see the following performance changes and no<br>
> change<br>
> > A A for all others.<br>
> > A A 164.gzip (ref) A +1.76%<br>
> > A A 458.sjeng (train) + 2.19%<br>
> > A A 471.omnetpp (train) -1.43%<br>
> > A A 473.astar (train) -1.51%<br>
> > A A Hopefully we can understand why this could happen, but maybe this<br>
> is just<br>
> > A A a heuristic result depending on the real control flow and<br>
> workload.<br>
<div>> > A A Thanks,<br>
> > A A -Jiangning<br>
> ><br>
</div>> > A A 2014-07-24 16:17 GMT+08:00 Sergey Dmitrouk<br>
> <<a href="mailto:sdmitrouk@accesssoftek.com" target="_blank">sdmitrouk@accesssoftek.com</a>>:<br>
> ><br>
> > A A A Hello Jiangning,<br>
> ><br>
> > A A A Thanks for your comments.<br>
> ><br>
> > A A A > 1) I expect the fix should be insideA getAArch64Cmp.<br>
> > A A A >...<br>
> > A A A > but essentially getAArch64Cmp missed the case of (x < 1) -><br>
> (x <= 0).<br>
> ><br>
> > A A A It was initial placement of the fix, but the function doesn't<br>
> seem to<br>
> > A A A perform transformations like that. A It updates conditions<br>
> only when<br>
> > A A A immediate values are not legal. A There is no comment for the<br>
> function,<br>
> > A A A so I'm not sure whether such checks fit there, but I moved the<br>
> change.<br>
> > A A A > 2) Your comment is inconsistent with your code.<br>
> ><br>
> > A A A Thanks, it's probably because of inverted conditions in DAGs.<br>
<div>> > A A A > So now I'm wondering how to justify this is always<br>
> meaningful for<br>
</div>> > A A A AArch64?<br>
> ><br>
> > A A A I wasn't sure whether it's worth such change, but as an option<br>
> something<br>
> > A A A like TargetLowering::isInversionBeneficial(SDValue Cond) can<br>
> be added,<br>
> > A A A but I don't know whether it's possible to check for conditions<br>
> like<br>
> > A A A "(a < 0 && b == c || a > 0 && b == d)" to do not block<br>
> inversion for all<br>
> > A A A cases.<br>
> ><br>
> > A A A Attached updated patch at least to see whether the fix fits<br>
> well in<br>
> > A A A getAArch64Cmp().<br>
> ><br>
> > A A A Regards,<br>
> > A A A Sergey<br>
> > A A A On Wed, Jul 23, 2014 at 10:05:37PM -0700, Jiangning Liu wrote:<br>
> > A A A > A A A Hi Sergey,<br>
> > A A A > A A A 1) I expect the fix should be insideA getAArch64Cmp.<br>
> > A A A > A A A 2) Your comment is inconsistent with your code. Your<br>
> code is to<br>
> > A A A transform<br>
> > A A A > A A A (x < 1) to be (x<=0), rather than "Turn "x > 1"<br>
> condition into "x<br>
> > A A A >= 0"".<br>
> > A A A > A A A I also noticed we have the following transformation<br>
> for if<br>
> > A A A condition (x <<br>
> > A A A > A A A 0) in back-end,<br>
> > A A A > A A A stage 1: (x < 0) -> (x >= 0), i.e. (x<0) and invert<br>
> the targets.<br>
> > A A A > A A A stage 2: (x >= 0) -> (x > -1). This happens in<br>
> combine1.<br>
> > A A A > A A A stage 3: (x > -1) -> (x >= 0) in getAArch64Cmp.<br>
> > A A A > A A A For if condition (x > 0), the transformation is<br>
> similar. Your<br>
> > A A A patch is<br>
> > A A A > A A A trying to cover this case, but essentially<br>
> getAArch64Cmp missed<br>
> > A A A the case<br>
> > A A A > A A A of (x < 1) -> (x <= 0).<br>
> > A A A > A A A However, as you can see the root cause of generating<br>
> the<br>
> > A A A comparison with<br>
> > A A A > A A A constant 1 is stage 1. This happens<br>
> > A A A > A A A insideA SelectionDAGBuilder::visitSwitchCase<br>
> > A A A > A A A A A // If the lhs block is the next block, invert the<br>
> condition<br>
> > A A A so that we<br>
> > A A A > A A A can<br>
> > A A A > A A A A A // fall through to the lhs instead of the rhs<br>
> block.<br>
> > A A A > A A A A A if (CB.TrueBB == NextBlock) {<br>
> > A A A > A A A A A A A std::swap(CB.TrueBB, CB.FalseBB);<br>
> > A A A > A A A A A A A SDValue True = DAG.getConstant(1,<br>
> Cond.getValueType());<br>
> > A A A > A A A A A A A Cond = DAG.getNode(ISD::XOR, dl,<br>
> Cond.getValueType(),<br>
> > A A A Cond, True);<br>
> > A A A > A A A A A }<br>
<div>> > A A A > A A A So now I'm wondering how to justify this is always<br>
> meaningful for<br>
</div>> > A A A AArch64?<br>
> > A A A > A A A Thanks,<br>
> > A A A > A A A -Jiangning<br>
> > A A A ><br>
> > A A A > A A A 2014-07-23 23:54 GMT+08:00 Sergey Dmitrouk<br>
> > A A A <<a href="mailto:sdmitrouk@accesssoftek.com" target="_blank">sdmitrouk@accesssoftek.com</a>>:<br>
> > A A A ><br>
> > A A A > A A A A A Hi,<br>
> > A A A ><br>
> > A A A > A A A A A Basing on the following information from [this<br>
> post][0] by<br>
> > A A A James Molloy:<br>
> > A A A ><br>
> > A A A > A A A A A A A 2. "if (a < 0 && b == c || a > 0 && b == d)" -<br>
> the first<br>
> > A A A comparison<br>
> > A A A > A A A A A of<br>
> > A A A > A A A A A A A 'a' against zero is done twice, when the flag<br>
> results of<br>
> > A A A the first<br>
> > A A A > A A A A A A A comparison could be used for the second<br>
> comparison.<br>
> > A A A ><br>
> > A A A > A A A A A I've made a patch (attached) that removes this<br>
> extra<br>
> > A A A comparison. A More<br>
> > A A A > A A A A A complex cases like comparisons with non-zero<br>
> immediate values<br>
> > A A A or with<br>
> > A A A > A A A A A registers doesn't seem to be task for a code<br>
> generator. A<br>
> > A A A Comparing with<br>
> > A A A > A A A A A zero is quite common, so I seems to be worth<br>
> adding.<br>
> > A A A ><br>
> > A A A > A A A A A Please review the patch. A Couldn't find a better<br>
> place to<br>
> > A A A make the<br>
> > A A A > A A A A A change, but I'll be happy to adjust the patch if<br>
> anyone has<br>
> > A A A better<br>
> > A A A > A A A A A ideas.<br>
> > A A A ><br>
> > A A A > A A A A A Best regards,<br>
> > A A A > A A A A A Sergey<br>
> > A A A ><br>
> > A A A > A A A A A 0:<br>
> > A A A <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>
> > A A A ><br>
> > A A A > A A A A A _______________________________________________<br>
> > A A A > A A A A A llvm-commits mailing list<br>
> > A A A > A A A A A <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> > A A A > A A A A 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></div></div><br></div>
</blockquote></div><br></div>