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

[Sergey Dmitrouk]

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