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

[Jiangning Liu]

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.  Returns true if compare instruction was changed, otherwise false
is
+// returned.

s/instraction/instruction/

+  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.

+    if (((HeadCmp == AArch64CC::GT && TrueCmp == AArch64CC::LT) ||
+         (HeadCmp == AArch64CC::LT && TrueCmp == AArch64CC::GT)) &&
+        std::abs(TrueImm - HeadImm) == 2) {
+      // This branch handles the following cases ({Imm} is the same on both
+      // sides):
+      // (a < {Imm} && ...) || (a > {Imm} && ...)
+      // (a > {Imm} && ...) || (a < {Imm} && ...)

Can you change the comments clarifying at this moment the Machine
Instructions are like

      // (a > {TrueImm} && ...) || (a < {HeadImm} && ...)
      // (a < {TrueImm} && ...) || (a > {HeadImm} && ...)
      //
      // and we want to change it to be like,
      //
      // (a >= {NewImm} && ...) || (a <= {NewImm} && ...)
      // (a <= {NewImm} && ...) || (a >= {NewImm} && ...)

+    } else if (((HeadCmp == AArch64CC::GT && TrueCmp == AArch64CC::GT) ||
+                (HeadCmp == AArch64CC::LT && TrueCmp == AArch64CC::LT)) &&
+                std::abs(TrueImm - HeadImm) == 1) {
+      // This branch handles the following cases ({Imm} is the same on both
+      // sides):
+      // (a < {Imm} && ...) || (a <= {Imm} && ...)
+      // (a > {Imm} && ...) || (a >= {Imm} && ...)

Can you change the comments clarifying at this moment the Machine
Instructions are like

      // (a > {TrueImm} && ...) || (a > {HeadImm} && ...)
      // (a < {TrueImm} && ...) || (a < {HeadImm} && ...)
      //
      // and we want to change it to be like,
      //
      // (a <= {NewImm} && ...) || (a > {NewImm} && ...)
      // (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.  I made some more changes that make transformations
> safer and skips them if there is no potential benefit.  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:
> >    Hi Sergey,
> >    Thanks for your update!
> >    Unfortunately, with the latest patch, I can see "miscompare" failures
> for
> >    445.gobmk and 482.sphinx3 in spec2006.
> >    Can you verify it in your local box?
> >    I think you can "run" them with qemu following
> >    http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-April/072085.html,
> if you
> >    don't really have aarch64 hardware.
> >    Thanks,
> >    -Jiangning
> >
> >    2014-08-13 16:51 GMT+08:00 Sergey Dmitrouk <
> sdmitrouk at accesssoftek.com>:
> >
> >      Hi Jiangning,
> >      > The 2nd compare instruction should not be under the same
> condition as
> >      the
> >      > 1st one, I think.
> >
> >      It should for currently supported transformations, otherwise we can
> try
> >      to adjust wrong instruction.A  Allowing second compare be followed
> by
> >      arbitrary instruction is a separate case that needs changes in at
> least
> >      two places (need to take into account that we can't touch it).A  I
> can
> >      do
> >      this if you want to have it now, but it'll increase size of the
> patch
> >      even more, so I just added TODO item for it at the top for now.
> >      >A  A  Can you try attached small case? With your patch, it may
> trigger
> >      >A  A  segmentation fault.
> >
> >      I missed handling of "cmp; ...; b.??" in adjustCmp(), it assumed the
> >      next instruction to be Bcc.A  Thank you, I reduced that code and
> added
> >      it to tests.A  I also added hidden command-line option to
> enable/disable
> >      the pass to omit recompilation just to disable it.
> >
> >      Thanks,
> >      Sergey
> >      On Tue, Aug 12, 2014 at 06:20:20PM -0700, Jiangning Liu wrote:
> >      >A  A  Hi Sergey,
> >      >A  A  The 2nd compare instruction should not be under the same
> >      condition as the
> >      >A  A  1st one, I think.
> >      >A  A  Can you try attached small case? With your patch, it may
> trigger
> >      >A  A  segmentation fault.
> >      >A  A  Thanks,
> >      >A  A  -Jiangning
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140825/7119e135/attachment.html>