[PATCH][AArch64] Enable sign check optimization by CSE
Jiangning Liu
liujiangning1 at gmail.com
Mon Aug 11 20:02:40 PDT 2014
Hi Sergey,
I looked at your code again, and I believe this piece of code is incorrect,
+ // Change immediate in comparison instruction (ADDS or SUBS).
+ MachineInstr *MI = BuildMI(*MBB, CmpMI, CmpMI->getDebugLoc(),
TII->get(Opc))
+ .addOperand(CmpMI->getOperand(0))
+ .addOperand(CmpMI->getOperand(1))
+ .addImm(imm + correction)
+ .addOperand(CmpMI->getOperand(3));
+ CmpMI->eraseFromParent();
+
+ MachineInstr *BrMI = MI->getNextNode();
+
+ // Change condition in branch instruction.
+ BuildMI(*MBB, BrMI, BrMI->getDebugLoc(), TII->get(AArch64::Bcc))
+ .addImm(getAdjustedCmp(Cmp))
+ .addOperand(BrMI->getOperand(1));
+ BrMI->eraseFromParent();
Here BrMI may not really be a branch instruction. You need to check this
before entering adjustCmp.
Thanks,
-Jiangning
2014-08-12 8:04 GMT+08:00 Hal Finkel <hfinkel at anl.gov>:
> ----- 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140812/e0bf9a63/attachment.html>
More information about the llvm-commits
mailing list