[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]

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.   

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