[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/Hexagon/

[Hal Finkel]

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.

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.

>     // 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.

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.

>   // 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:

  // 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.

 -Hal

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory