[llvm-commits] [llvm] r146412 [1/2] - in /llvm/trunk: ./ autoconf/ docs/ include/llvm/ include/llvm/ADT/ include/llvm/Support/ lib/Support/ lib/Target/Hexagon/ lib/Target/Hexagon/TargetInfo/ projects/sample/ projects/sample/autoconf/ test/CodeGen/Hexa
Hal Finkel
hfinkel at anl.gov
Fri Jun 29 17:06:57 PDT 2012
On Fri, 29 Jun 2012 17:51:14 -0500
"Brendon Cahoon" <bcahoon at codeaurora.org> wrote:
> Hi Hal,
>
> I apologize for the delay in getting back to you. I've been trying
> to find time to get our version up-to-date with the LLVM mainline.
> I'll carve out some time next week to get our version in sync with
> the LLVM code, and updated with your comments/changes. Is that OK?
Sounds great!
-Hal
>
> Again, sorry for the delay.
>
> Thanks,
> Brendon
>
> --
> Qualcomm Innovation Center, Inc is a member of Code Aurora Forum
>
> -----Original Message-----
> From: Hal Finkel [mailto:hfinkel at anl.gov]
> Sent: Monday, June 25, 2012 12:25 AM
> To: Hal Finkel
> Cc: Brendon Cahoon; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [llvm] r146412 [1/2] - in /llvm/trunk: ./
> autoconf/ docs/ include/llvm/ include/llvm/ADT/ include/llvm/Support/
> lib/Support/ lib/Target/Hexagon/ lib/Target/Hexagon/TargetInfo/
> projects/sample/ projects/sample/autoconf/ test/CodeGen/Hexa
>
> On Fri, 8 Jun 2012 14:33:56 -0500
> Hal Finkel <hfinkel at anl.gov> wrote:
>
> > On Fri, 8 Jun 2012 14:14:58 -0500
> > "Brendon Cahoon" <bcahoon at codeaurora.org> wrote:
> >
> > > Hi Hal,
> > >
> > > Great to see that you were able to reuse the code for PowerPC.
> > > I've made some changes to the Hexagon version, which I need to
> > > try to upstream. Also, you've made some good suggestions below.
> > > As Chris suggested, it would be great to work towards a version
> > > that shared the common code.
> > >
> > > The changes we made have been to enable more hardware loops cases.
> > > One example - the upstream code stops processing a loop after
> > > failing the create a hardware loop with the first induction
> > > variable it sees. Instead, all the induction variables need to be
> > > checked since the first may not be related to the loop count.
>
> This is now also implemented in the trunk PPC version, thanks! Do you
> know when you'll be able to get a more-recent version of the Hexagon
> version upstream (or at least send us a copy)?
>
> -Hal
>
> >
> > Brendon,
> >
> > It would be great if you could get those changes upstream. As I
> > said in response to Chris's e-mail, if we can synchronize
> > functionality, then I think that abstracting the differences will
> > be relatively easy. I would be happy to work with you on this.
> >
> > >
> > > We're still not catching some cases that I would really like to
> > > identify. I've seen a case where the code is changed by a prior
> > > pass such that the loop end compare uses the source register of
> > > the induction expression (and updates the end condition
> > > accordingly). The existing hardware loop code expects that it's
> > > the induction expression destination that feeds into the compare.
> >
> > Interesting. Yes, we should work on catching those as well.
> >
> > >
> > > I've added some additional comments below. Thanks for your
> > > comments!
> > >
> > > -- Brendon
> > >
> > > --
> > > Qualcomm Innovation Center, Inc is a member of Code Aurora Forum
> > >
> > > -----Original Message-----
> > > From: llvm-commits-bounces at cs.uiuc.edu
> > > [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Hal Finkel
> > > Sent: Friday, June 08, 2012 11:39 AM
> > > To: Tony Linthicum
> > > Cc: llvm-commits at cs.uiuc.edu
> > > Subject: Re: [llvm-commits] [llvm] r146412 [1/2] -
> > > in /llvm/trunk: ./ autoconf/ docs/ include/llvm/
> > > include/llvm/ADT/ include/llvm/Support/ lib/Support/
> > > lib/Target/Hexagon/ lib/Target/Hexagon/TargetInfo/
> > > projects/sample/ projects/sample/autoconf/ test/CodeGen/Hexa
> > >
> > > On Mon, 12 Dec 2011 21:14:41 -0000
> > > Tony Linthicum <tlinth at codeaurora.org> wrote:
> > >
> > > > HexagonHardware
> > >
> > > I just checked in a PPCCTRLoops pass which is heavily based on
> > > your HexagonHardwareLoops pass. I made some improvements over the
> > > original (such as deleting unneeded comparison and induction
> > > variables) that you might want to incorporate into the Hexagon
> > > version.
> > >
> > > [Brendon]
> > > This improvement is great. I'll definitely look to add this code
> > > to the Hexagon version. We've changed the Machine DCE pass to do
> > > this, but that requires the pass to be run again after the
> > > hardware loop pass.
> >
> > Neat!
> >
> > >
> > > Other specific comments on the HexagonHardwareLoops pass:
> > >
> > > > bool
> > > > HexagonHardwareLoops::isInductionOperation(const MachineInstr
> > > > *MI, unsigned IVReg) const {
> > > > return (MI->getOpcode() ==
> > > > Hexagon::ADD_ri && MI->getOperand(1).getReg() ==
> > > > IVReg);
> > >
> > > In the PPC version I needed to add a check on
> > > MI->getOperand(1).isReg() because there may also be adds with a
> > > frame index instead of a register at this point in the pipeline.
> > > You might need to do the same.
> > >
> > > [Brendon]
> > > We will always see a register here with our ADD_ri instruction,
> > > but certainly checking for a register is a good idea.
> > >
> > > > // Add the Loop instruction to the beginning of the loop.
> > > > BuildMI(*Preheader, InsertPos, InsertPos->getDebugLoc(),
> > > >
> > > > TII->get(Hexagon::LOOP0_r)).addMBB(LoopStart).addReg(CountReg);
> > >
> > > There are a number of places in this function where
> > > InsertPos->getDebugLoc() is used. I discovered there are problems
> > > with this when inserting at the very end of the block. I do this
> > > instead:
> > >
> > > DebugLoc dl;
> > > if (InsertPos != Preheader->end())
> > > dl = InsertPos->getDebugLoc();
> > >
> > > and then use dl instead.
> > >
> > > [Brendon]
> > > Thanks.
> > >
> > > Also, while technically not a problem here (because MachineDCE
> > > runs prior to this pass), I tested running MachineDCE afterward,
> > > and discovered that I needed to set the LiveIn registers on the
> > > non-preheader loop blocks manually (otherwise the 'begin loop'
> > > instruction would get deleted). It seems sufficient to set the
> > > LiveIn for the LastMBB (SA0, LC0 in your case), but I'm currently
> > > setting them on all other blocks as well. In the name of
> > > future-proofing, I suggest that you also do this.
> > >
> > > [Brendon]
> > > I'll investigate this some more. I'm not sure why we don't see a
> > > problem currently. Maybe it's because the ENDLOOP instruction
> > > uses these registers?
> >
> > I am not sure either. The BDNZ instruction (which is analogous to
> > your ENDLOOP instruction) also uses the associated registers.
> >
> > Thanks again,
> > Hal
> >
> > >
> > > > // The loop ends with either:
> > > > // - a conditional branch followed by an unconditional
> > > > branch, or // - a conditional branch to the loop start.
> > > > if (LastI->getOpcode() == Hexagon::JMP_c ||
> > > > LastI->getOpcode() == Hexagon::JMP_cNot) {
> > > > // delete one and change/add an uncond. branch to out of
> > > > the loop MachineBasicBlock *BranchTarget =
> > > > LastI->getOperand(1).getMBB(); LastI = LastMBB->erase(LastI);
> > > > if (!L->contains(BranchTarget)) {
> > > > if (LastI != LastMBB->end()) {
> > > > TII->RemoveBranch(*LastMBB);
> > > > }
> > > > SmallVector<MachineOperand, 0> Cond;
> > > > TII->InsertBranch(*LastMBB, BranchTarget, 0, Cond, dl);
> > > > }
> > > > } else {
> > > > // Conditional branch to loop start; just delete it.
> > > > LastMBB->erase(LastI);
> > > > }
> > >
> > > while I believe that the comment is correct, I don't really
> > > understand the code. If nothing else, please add an assert in the
> > > 'else' condition so that it is clear what is going on (it seems
> > > like the 'if' catches the conditional branches, so I'm not sure
> > > what is being otherwise deleted). I have the following in my PPC
> > > version:
> > >
> > > [Brendon]
> > > Yeah - that comment doesn't make sense to me either.
> > >
> > > // The loop ends with either:
> > > // - a conditional branch followed by an unconditional branch,
> > > or // - a conditional branch to the loop start.
> > > assert(LastI->getOpcode() == PPC::BCC &&
> > > "loop end must start with a BCC instruction");
> > > // Either the BCC branches to the beginning of the loop, or it
> > > // branches out of the loop and there is an unconditional branch
> > > // to the start of the loop.
> > > MachineBasicBlock *BranchTarget = LastI->getOperand(2).getMBB();
> > > BuildMI(*LastMBB, LastI, dl,
> > > TII->get((BranchTarget == LoopStart) ?
> > > (isPPC64 ? PPC::BDNZ8 : PPC::BDNZ) :
> > > (isPPC64 ? PPC::BDZ8 :
> > > PPC::BDZ))).addMBB(BranchTarget);
> > >
> > > // Conditional branch; just delete it.
> > > LastMBB->erase(LastI);
> > >
> > > And a final note, you may wish to investigate adding support for
> > > ENDLOOP to your AnalyzeBranch function in HexagonInstrInfo. Doing
> > > so will allow block-placement to optimize the BB ordering, and
> > > remove unneeded unconditional branches.
> > >
> > > [Brendon]
> > > Yep. We have added logic to AnalyzeBranch to get rid of
> > > unnecessary unconditional branches, but we've also recently added
> > > support for ENDLOOP so we may be able to remove the first change.
> > > Thanks.
> > >
> > > -Hal
> > >
> > > --
> > > Hal Finkel
> > > Postdoctoral Appointee
> > > Leadership Computing Facility
> > > Argonne National Laboratory
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > >
> >
> >
> >
>
>
>
> --
> Hal Finkel
> Postdoctoral Appointee
> Leadership Computing Facility
> Argonne National Laboratory
>
--
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list