[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
Sun Jun 24 22:24:56 PDT 2012


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



More information about the llvm-commits mailing list