[PATCH] D154205: [MachineLICM] Handle subloops

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

In D154205#4494458 <https://reviews.llvm.org/D154205#4494458>, @nikic wrote:

> In D154205#4494166 <https://reviews.llvm.org/D154205#4494166>, @craig.topper wrote:
>
>> My question wasn't about visiting the inner loops it was about the order of visiting. Since I think it visits all blocks in child loops, I was wondering if we could aggressively hoist everything relative to the outer loop first instead of gradually. Then visit the inner loops to get anything that can't be hoisted all the way out. Though if we do as @nikic says and stop visiting blocks of inner loops when visiting outer loops, this won't work.
>
> There are multiple ways to go about this. We could only visit the outermost loop, but hoist to an inner loop preheader while doing so (i.e. perform the invariance check against the current inner-most loop to find candidates, and then hoist them to the outer-most parent loop that is still invariant). It's possible that doing this fits better into the MachineLICM model, which also needs to keep track of register pressure.

Yep, I think it is good idea.
As below patch, I tried to skip the blocks which have already visited with inner loops but the different register pressure caused regression. In order to keep track of the register pressure properly, we need to visit all blocks in loop again...
Maybe, your idea could avoid the issue with keeping track of the register pressure properly...

  diff --git a/llvm/lib/CodeGen/MachineLICM.cpp b/llvm/lib/CodeGen/MachineLICM.cpp
  index 1b84c0218867..c7ab88a3af22 100644
  --- a/llvm/lib/CodeGen/MachineLICM.cpp
  +++ b/llvm/lib/CodeGen/MachineLICM.cpp
  @@ -239,9 +239,11 @@ namespace {
       void ExitScopeIfDone(
           MachineDomTreeNode *Node,
           DenseMap<MachineDomTreeNode *, unsigned> &OpenChildren,
  -        const DenseMap<MachineDomTreeNode *, MachineDomTreeNode *> &ParentMap);
  +        const DenseMap<MachineDomTreeNode *, MachineDomTreeNode *> &ParentMap,
  +        SmallPtrSetImpl<MachineBasicBlock *> &VisitedLoopBBs);
   
  -    void HoistOutOfLoop(MachineDomTreeNode *HeaderN);
  +    void HoistOutOfLoop(MachineDomTreeNode *HeaderN,
  +                        SmallPtrSetImpl<MachineBasicBlock *> &VisitedLoopBBs);
   
       void InitRegPressure(MachineBasicBlock *BB);
   
  @@ -375,6 +377,7 @@ bool MachineLICMBase::runOnMachineFunction(MachineFunction &MF) {
     for (; MLII != MLIE; ++MLII)
       addSubLoopsToWorkList(*MLII, Worklist, PreRegAlloc);
   
  +  SmallPtrSet<MachineBasicBlock *, 32> VisitedLoopBBs;
     while (!Worklist.empty()) {
       CurLoop = Worklist.pop_back_val();
       CurPreheader = nullptr;
  @@ -389,8 +392,11 @@ bool MachineLICMBase::runOnMachineFunction(MachineFunction &MF) {
         // being hoisted.
         MachineDomTreeNode *N = DT->getNode(CurLoop->getHeader());
         FirstInLoop = true;
  -      HoistOutOfLoop(N);
  +      HoistOutOfLoop(N, VisitedLoopBBs);
         CSEMap.clear();
  +      // Keep track of visited loop's blocks.
  +      VisitedLoopBBs.insert(CurLoop->getBlocksVector().begin(),
  +                            CurLoop->getBlocksVector().end());
       }
     }
   
  @@ -694,14 +700,18 @@ void MachineLICMBase::ExitScope(MachineBasicBlock *MBB) {
   /// Destroy scope for the MBB that corresponds to the given dominator tree node
   /// if its a leaf or all of its children are done. Walk up the dominator tree to
   /// destroy ancestors which are now done.
  -void MachineLICMBase::ExitScopeIfDone(MachineDomTreeNode *Node,
  -    DenseMap<MachineDomTreeNode*, unsigned> &OpenChildren,
  -    const DenseMap<MachineDomTreeNode*, MachineDomTreeNode*> &ParentMap) {
  +void MachineLICMBase::ExitScopeIfDone(
  +    MachineDomTreeNode *Node,
  +    DenseMap<MachineDomTreeNode *, unsigned> &OpenChildren,
  +    const DenseMap<MachineDomTreeNode *, MachineDomTreeNode *> &ParentMap,
  +    SmallPtrSetImpl<MachineBasicBlock *> &VisitedLoopBBs) {
     if (OpenChildren[Node])
       return;
   
  -  for(;;) {
  -    ExitScope(Node->getBlock());
  +  for (;;) {
  +    // If the block was visited previously, do not process the block.
  +    if (!VisitedLoopBBs.contains(Node->getBlock()))
  +      ExitScope(Node->getBlock());
       // Now traverse upwards to pop ancestors whose offsprings are all done.
       MachineDomTreeNode *Parent = ParentMap.lookup(Node);
       if (!Parent || --OpenChildren[Parent] != 0)
  @@ -714,7 +724,9 @@ void MachineLICMBase::ExitScopeIfDone(MachineDomTreeNode *Node,
   /// specified header block, and that are in the current loop) in depth first
   /// order w.r.t the DominatorTree. This allows us to visit definitions before
   /// uses, allowing us to hoist a loop body in one pass without iteration.
  -void MachineLICMBase::HoistOutOfLoop(MachineDomTreeNode *HeaderN) {
  +void MachineLICMBase::HoistOutOfLoop(
  +    MachineDomTreeNode *HeaderN,
  +    SmallPtrSetImpl<MachineBasicBlock *> &VisitedLoopBBs) {
     MachineBasicBlock *Preheader = getCurPreheader();
     if (!Preheader)
       return;
  @@ -741,7 +753,10 @@ void MachineLICMBase::HoistOutOfLoop(MachineDomTreeNode *HeaderN) {
       if (!CurLoop->contains(BB))
         continue;
   
  -    Scopes.push_back(Node);
  +    // If the block was visited previously, do not process the block.
  +    if (VisitedLoopBBs.empty() || !VisitedLoopBBs.contains(BB))
  +      Scopes.push_back(Node);
  +
       unsigned NumChildren = Node->getNumChildren();
   
       // Don't hoist things out of a large switch statement.  This often causes
  @@ -786,7 +801,7 @@ void MachineLICMBase::HoistOutOfLoop(MachineDomTreeNode *HeaderN) {
       }
   
       // If it's a leaf node, it's done. Traverse upwards to pop ancestors.
  -    ExitScopeIfDone(Node, OpenChildren, ParentMap);
  +    ExitScopeIfDone(Node, OpenChildren, ParentMap, VisitedLoopBBs);
     }
   }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154205/new/

https://reviews.llvm.org/D154205