[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

[Brendon Cahoon]

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.   

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.

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.

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?

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