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