[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 7:31:47 AM
> Subject: Re: [PATCH][AArch64] Enable sign check optimization by CSE
> 
> Thanks, changed that part to:
> 
>     Due to the way selection DAG part that corresponds to switch case
>     is built,

I recommend something like this: "As a result of the canonicalization employed by SelectionDAGBuilder::visitSwitchCase, DAGCombine, and other target-specific code, "

> 
> Jiangning gave a nice overview of what really happens here:
> 
>     http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140721/227116.html
> 
> I'm not sure whether CodeGenPrep is able to perform transformations
> like
> this one.

Probably not. Based on Jiangning's explanation, the SDAG builder's canonicalization, and later code, is at fault here.

 -Hal

> 
> Cheers,
> Sergey
> 
> On Mon, Aug 11, 2014 at 04:56:25AM -0700, Hal Finkel wrote:
> > ----- 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
> 

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