[PATCH][AArch64] Enable sign check optimization by CSE

[Jiangning Liu]

Hi Sergey,

Did you forget to attach your new patch?

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.

164.gzip (ref)  +1.76%
458.sjeng (train) + 2.19%
471.omnetpp (train) -1.43%
473.astar (train) -1.51%

Hopefully we can understand why this could happen, but maybe this is just a
heuristic result depending on the real control flow and workload.

Thanks,
-Jiangning



2014-07-24 16:17 GMT+08:00 Sergey Dmitrouk <sdmitrouk at accesssoftek.com>:

> Hello Jiangning,
>
> Thanks for your comments.
>
> > 1) I expect the fix should be insideA getAArch64Cmp.
> >...
> > but essentially getAArch64Cmp missed the case of (x < 1) -> (x <= 0).
>
> It was initial placement of the fix, but the function doesn't seem to
> perform transformations like that.  It updates conditions only when
> immediate values are not legal.  There is no comment for the function,
> so I'm not sure whether such checks fit there, but I moved the change.
>
> > 2) Your comment is inconsistent with your code.
>
> Thanks, it's probably because of inverted conditions in DAGs.
>
> > So now I'm wondering how to justify this is always meaningful for
> AArch64?
>
> I wasn't sure whether it's worth such change, but as an option something
> like TargetLowering::isInversionBeneficial(SDValue Cond) can be added,
> but I don't know whether it's possible to check for conditions like
> "(a < 0 && b == c || a > 0 && b == d)" to do not block inversion for all
> cases.
>
> Attached updated patch at least to see whether the fix fits well in
> getAArch64Cmp().
>
> Regards,
> Sergey
>
> On Wed, Jul 23, 2014 at 10:05:37PM -0700, Jiangning Liu wrote:
> >    Hi Sergey,
> >    1) I expect the fix should be insideA getAArch64Cmp.
> >    2) Your comment is inconsistent with your code. Your code is to
> transform
> >    (x < 1) to be (x<=0), rather than "Turn "x > 1" condition into "x >=
> 0"".
> >    I also noticed we have the following transformation for if condition
> (x <
> >    0) in back-end,
> >    stage 1: (x < 0) -> (x >= 0), i.e. (x<0) and invert the targets.
> >    stage 2: (x >= 0) -> (x > -1). This happens in combine1.
> >    stage 3: (x > -1) -> (x >= 0) in getAArch64Cmp.
> >    For if condition (x > 0), the transformation is similar. Your patch is
> >    trying to cover this case, but essentially getAArch64Cmp missed the
> case
> >    of (x < 1) -> (x <= 0).
> >    However, as you can see the root cause of generating the comparison
> with
> >    constant 1 is stage 1. This happens
> >    insideA SelectionDAGBuilder::visitSwitchCase
> >    A  // If the lhs block is the next block, invert the condition so
> that we
> >    can
> >    A  // fall through to the lhs instead of the rhs block.
> >    A  if (CB.TrueBB == NextBlock) {
> >    A  A  std::swap(CB.TrueBB, CB.FalseBB);
> >    A  A  SDValue True = DAG.getConstant(1, Cond.getValueType());
> >    A  A  Cond = DAG.getNode(ISD::XOR, dl, Cond.getValueType(), Cond,
> True);
> >    A  }
> >    So now I'm wondering how to justify this is always meaningful for
> AArch64?
> >    Thanks,
> >    -Jiangning
> >
> >    2014-07-23 23:54 GMT+08:00 Sergey Dmitrouk <
> sdmitrouk at accesssoftek.com>:
> >
> >      Hi,
> >
> >      Basing on the following information from [this post][0] by James
> Molloy:
> >
> >      A  2. "if (a < 0 && b == c || a > 0 && b == d)" - the first
> comparison
> >      of
> >      A  'a' against zero is done twice, when the flag results of the
> first
> >      A  comparison could be used for the second comparison.
> >
> >      I've made a patch (attached) that removes this extra comparison. A
> More
> >      complex cases like comparisons with non-zero immediate values or
> with
> >      registers doesn't seem to be task for a code generator. A Comparing
> with
> >      zero is quite common, so I seems to be worth adding.
> >
> >      Please review the patch. A Couldn't find a better place to make the
> >      change, but I'll be happy to adjust the patch if anyone has better
> >      ideas.
> >
> >      Best regards,
> >      Sergey
> >
> >      0: http://article.gmane.org/gmane.comp.compilers.llvm.devel/74269
> >
> >      _______________________________________________
> >      llvm-commits mailing list
> >      llvm-commits at cs.uiuc.edu
> >      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/20140725/c8cd2946/attachment.html>