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