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

[Hal Finkel]

----- Original Message -----
> From: "Sergey Dmitrouk" <sdmitrouk at accesssoftek.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>, "Jiangning Liu" <liujiangning1 at gmail.com>
> Sent: Monday, August 11, 2014 3:29:34 AM
> Subject: Re: [PATCH][AArch64] Enable sign check optimization by CSE
> 
> Hi Hal,
> 
> thanks for your comments.  getNexeNode() works.  I added more verbose
> comment at the top and examples in C after conditions.  I didn't
> provide
> more detailed examples for the later as it would repeat the top
> comment.
> Changes of instructions are quite straightforward once you get the
> idea
> that value is changed by one and comparison inclusive/exclusive type
> is
> switched.  Still can add it if you prefer, just don't want to repeat
> same information in several places.

Thanks, these comments are much better.

+// Unfortunately, due to they way other parts implemented, corresponding

parts implemented -> parts are implemented

Also, what other parts? Specifically, does this happen at the SelectionDAG stage or later? If not, I think this logic would be better in CodeGenPrep instead of here.

 -Hal

> 
> Updated patch is attached.
> 
> Regards,
> Sergey
> 
> On Sun, Aug 10, 2014 at 05:32:57PM -0700, Hal Finkel wrote:
> > 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
> 

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