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

[Hal Finkel]

Hi Sergey,

+  MachineInstr *BrMI = &*std::next(MachineBasicBlock::iterator(MI));

I think this can be: MachineInstr *BrMI = MI->getNextNode();

+    if (((HeadCmp == AArch64CC::GT && TrueCmp == AArch64CC::LT) ||
+          (HeadCmp == AArch64CC::LT && TrueCmp == AArch64CC::GT)) &&
+        std::abs(TrueImm - HeadImm) == 2) {
+      adjustCmp(HeadCmpMI, HeadCmp);
+      adjustCmp(TrueCmpMI, TrueCmp);
+      Changed = true;
+    } else if (((HeadCmp == AArch64CC::GT && TrueCmp == AArch64CC::GT) ||
+                (HeadCmp == AArch64CC::LT && TrueCmp == AArch64CC::LT)) &&
+                std::abs(TrueImm - HeadImm) == 1) {
+      if (HeadImm < TrueCmp) {
+        adjustCmp(HeadCmpMI, HeadCmp);
+      } else {
+        adjustCmp(TrueCmpMI, TrueCmp);
+      }
+      Changed = true;
+    }
+  }

I think some comments here would be helpful here giving an example (before and after) for each of these cases. You provide an example in C form in your e-mail below, but no such example appears in the comments in the patch (and relating the example at the instruction level would be good too).

Likewise, the comment at the top of the file should do at least as good a job at explaining what the pass does as your e-mail.

Thanks,
Hal

----- Original Message -----
> From: "Sergey Dmitrouk" <sdmitrouk at accesssoftek.com>
> To: "Jiangning Liu" <liujiangning1 at gmail.com>
> Cc: "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Sunday, August 10, 2014 1:25:38 PM
> Subject: Re: [PATCH][AArch64] Enable sign check optimization by CSE
> 
> Hi Jiangning,
> 
> Please find attached patch that adds new pass.  It analyzes branches
> and
> adjusts comparisons and conditions to make compare instructions look
> the
> same, so that CSE pass could remove them.  The patch is big, but half
> of
> it is test file (tried to make it simpler, but it leads to generating
> different assembly) and another half is the new pass, which is hard
> to
> break into smaller patches.  I think it demonstrates general
> idea (comments in test has comments in C): for code like
> 
>     if ((a > 5 && b == c) || (a >= 5 && b == d)) {
> 
> a is compared with 5 only once with the patch.
> 
> Currently only SUBS and ADDS followed by b.?? are supported.  If this
> patch is accepted, it should be possible to additionally:
>  * cover TBNZ/TBZ, which are now used for checks like (x < 0) and
>    doesn't let my patch optimize this case; does it make sense to use
>    one CMP instruction instead of TBNZ followed by CMP?
>  * handle other conditional instructions (e.g. CSET).
> 
> Comments are welcome, not sure I did it all correctly.  Maybe need to
> add command-line option for the pass, TODO comments, debug output,
> statistics or rewrite some parts completely.
> 
> Cheers,
> Sergey
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory