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

[Sergey Dmitrouk]

Hi Jiangning,

Thanks for the feedback.

> For large patch, I personally think it would be a good choice if we
> can use review.llvm.org.

Added as http://reviews.llvm.org/D5044 with added/fixed comments.

> I can't remember why AArch64CC::LE and AArch64CC::GE needn't to be
> covered at this stage. Do you have answer to that?

Combinations with AArch64CC::LE and AArch64CC::GE just didn't occur in
my tests.  Comparisons like "x <= y" and "x >= y" are lowered as
"x < y + 1" and "x > y - 1" accordingly.  Actually, after adjusting and
actual update of machine instructions have been splitted those checks
can be simplified to bruteforce (pseudo code):

    Changed |= try(adjust(Head), True) ||
               try(Head, adjust(True)) ||
               try(adjust(Head), adjust(True));

It should cover all possible cases and because of checks no meaningless
changes will be done.  It wasn't possible initially and I didn't bother
writing checks for something I couldn't find in generated code.  I can
do that if such bruteforce is acceptable (I don't remember such
approach applied in LLVM code I've seen so far).

Best regards,
Sergey

On Sun, Aug 24, 2014 at 08:26:04PM -0700, Jiangning Liu wrote:
>    Hi Sergey,
>    Thanks for your update! For large patch, I personally think it would be a
>    good choice if we can use review.llvm.org.
>    Overall I'm OK with the patch. There are some minor nitpicks.
>    +// Adjusts one cmp instraction to another one if result of adjustment
>    will allow
>    +// CSE. A Returns true if compare instruction was changed, otherwise
>    false is
>    +// returned.
>    s/instraction/instruction/
>    + A bool Negative = Opc == AArch64::ADDSWri || Opc == AArch64::ADDSXri;
>    Can you add comments describing why ADDS implies negative? And I prefer to
>    add brackets for entire right hand expression.
>    + A  A if (((HeadCmp == AArch64CC::GT && TrueCmp == AArch64CC::LT) ||
>    + A  A  A  A  (HeadCmp == AArch64CC::LT && TrueCmp == AArch64CC::GT)) &&
>    + A  A  A  A std::abs(TrueImm - HeadImm) == 2) {
>    + A  A  A // This branch handles the following cases ({Imm} is the same on
>    both
>    + A  A  A // sides):
>    + A  A  A // (a < {Imm} && ...) || (a > {Imm} && ...)
>    + A  A  A // (a > {Imm} && ...) || (a < {Imm} && ...)
>    Can you change the comments clarifying at this moment the Machine
>    Instructions are like
>    A  A  A  // (a > {TrueImm} && ...) || (a < {HeadImm} && ...)
>    A  A  A  // (a < {TrueImm} && ...) || (a > {HeadImm} && ...)
>    A  A  A  //
>    A  A  A  // and we want to change it to be like,
>    A  A  A  //
>    A  A  A  // (a >= {NewImm} && ...) || (a <= {NewImm} && ...)
>    A  A  A  // (a <= {NewImm} && ...) || (a >= {NewImm} && ...)
>    + A  A } else if (((HeadCmp == AArch64CC::GT && TrueCmp == AArch64CC::GT)
>    ||
>    + A  A  A  A  A  A  A  A (HeadCmp == AArch64CC::LT && TrueCmp ==
>    AArch64CC::LT)) &&
>    + A  A  A  A  A  A  A  A std::abs(TrueImm - HeadImm) == 1) {
>    + A  A  A // This branch handles the following cases ({Imm} is the same on
>    both
>    + A  A  A // sides):
>    + A  A  A // (a < {Imm} && ...) || (a <= {Imm} && ...)
>    + A  A  A // (a > {Imm} && ...) || (a >= {Imm} && ...)
>    Can you change the comments clarifying at this moment the Machine
>    Instructions are like
>    A  A  A  // (a > {TrueImm} && ...) || (a > {HeadImm} && ...)
>    A  A  A  // (a < {TrueImm} && ...) || (a < {HeadImm} && ...)
>    A  A  A  //
>    A  A  A  // and we want to change it to be like,
>    A  A  A  //
>    A  A  A  // (a <= {NewImm} && ...) || (a > {NewImm} && ...)
>    A  A  A  // (a < {NewImm} && ...) || (a >= {NewImm} && ...)
>    I know your original comments want to clarify the relationship to the C
>    source code, but it's really hard to understand how it is linked with
>    current Machine Instruction representation. If you still want to describe
>    this, I think the transformation in SelectionDAG part should probably be
>    articulated as well somewhere.
>    I can't remember why AArch64CC::LE and AArch64CC::GE needn't to be covered
>    at this stage. Do you have answer to that?
>    Thanks,
>    -Jiangning
> 
>    2014-08-22 2:10 GMT+08:00 Sergey Dmitrouk <sdmitrouk at accesssoftek.com>:
> 
>      Hi,
> 
>      Thanks, Jiangning.A  I made some more changes that make transformations
>      safer and skips them if there is no potential benefit.A  I run all test
>      cases for both apps, which show no regressions.
> 
>      The main patch is still named combine-comparisons-by-cse.patch, while
>      the second file (combine-comparisons-by-cse-changes.diff) contains
>      only the latest changes for convenience.
> 
>      Thanks,
>      Sergey
>      On Thu, Aug 14, 2014 at 10:14:31PM -0700, Jiangning Liu wrote:
>      >A  A  Hi Sergey,
>      >A  A  Thanks for your update!
>      >A  A  Unfortunately, with the latest patch, I can see "miscompare"
>      failures for
>      >A  A  445.gobmk and 482.sphinx3 in spec2006.
>      >A  A  Can you verify it in your local box?
>      >A  A  I think you can "run" them with qemu following
>      >A  A 
>      http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-April/072085.html, if
>      you
>      >A  A  don't really have aarch64 hardware.
>      >A  A  Thanks,
>      >A  A  -Jiangning
>      >
>      >A  A  2014-08-13 16:51 GMT+08:00 Sergey Dmitrouk
>      <sdmitrouk at accesssoftek.com>:
>      >
>      >A  A  A  Hi Jiangning,
>      >A  A  A  > The 2nd compare instruction should not be under the same
>      condition as
>      >A  A  A  the
>      >A  A  A  > 1st one, I think.
>      >
>      >A  A  A  It should for currently supported transformations, otherwise
>      we can try
>      >A  A  A  to adjust wrong instruction.AA  Allowing second compare be
>      followed by
>      >A  A  A  arbitrary instruction is a separate case that needs changes in
>      at least
>      >A  A  A  two places (need to take into account that we can't touch
>      it).AA  I can
>      >A  A  A  do
>      >A  A  A  this if you want to have it now, but it'll increase size of
>      the patch
>      >A  A  A  even more, so I just added TODO item for it at the top for
>      now.
>      >A  A  A  >AA  AA  Can you try attached small case? With your patch, it
>      may trigger
>      >A  A  A  >AA  AA  segmentation fault.
>      >
>      >A  A  A  I missed handling of "cmp; ...; b.??" in adjustCmp(), it
>      assumed the
>      >A  A  A  next instruction to be Bcc.AA  Thank you, I reduced that code
>      and added
>      >A  A  A  it to tests.AA  I also added hidden command-line option to
>      enable/disable
>      >A  A  A  the pass to omit recompilation just to disable it.
>      >
>      >A  A  A  Thanks,
>      >A  A  A  Sergey
>      >A  A  A  On Tue, Aug 12, 2014 at 06:20:20PM -0700, Jiangning Liu wrote:
>      >A  A  A  >AA  AA  Hi Sergey,
>      >A  A  A  >AA  AA  The 2nd compare instruction should not be under the
>      same
>      >A  A  A  condition as the
>      >A  A  A  >AA  AA  1st one, I think.
>      >A  A  A  >AA  AA  Can you try attached small case? With your patch, it
>      may trigger
>      >A  A  A  >AA  AA  segmentation fault.
>      >A  A  A  >AA  AA  Thanks,
>      >A  A  A  >AA  AA  -Jiangning