[PATCH] D41463: [CodeGen] Add a new pass for PostRA sink

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/CodeGen/MachineSink.cpp:995
+      continue;
+    for (const auto LI : SI->liveins())
+      if (AliasedRegs.count(LI.PhysReg))
----------------
sebpop wrote:
> junbuml wrote:
> > sebpop wrote:
> > > So at this point we are at a loop depth 4:
> > > 
> > >   for (BB : MF)
> > >     for (MI: BB)
> > >       for (SI : BB.successors)
> > >         for (LI : SI->liveins)
> > > 
> > > I think you could do the last two loops above the loop (MI:BB) by asking first whether there is an opportunity to perform the sinking from BB into one of the BB.successors.  The liveins are stored as a bitmap, and you could efficiently compute the difference of liveins in the successors. The registers in the diff are candidates for sinking.
> > > 
> > > Also please post compile time numbers with this change.
> > > 
> > > I think you could do the last two loops above the loop (MI:BB) by asking first whether there is an opportunity to perform the sinking from BB into one of the BB.successors.
> > 
> > We perform this loop only when we can find a single successor to which we can sink in the right above loop line 978.  Are you asking to move this check done in this loop outside of getSingleLiveInSuccBB() before finding the single sinkable successor ? 
> > 
> > > The liveins are stored as a bitmap, and you could efficiently compute the difference of liveins in the successors. The registers in the diff are candidates for sinking.
> > 
> > I'm not clear about this. As far as I checked, LiveIns in a MachineBasicBlock is a vector. I use a bitmap, but that is to track def/use of registers.  
> > 
> > > Also please post compile time numbers with this change.
> > 
> > I will share compile time info soon. 
> My reasoning is that we could compute the same information independently of an MI.
> By only looking at a BB and the liveins in its successors, we can compute whether there is an opportunity to sink.
> 
I think you meant that we first try to find sinkable Regs in advance independently of an MI and iterate MIs only when there is candidates, right? 

Assuming that this was what you meant, I doubt if doing so in advance is beneficial for the compile time.  To be independent on MI, we need to do this aliased reg check for all live-ins in successors of every BB.  

In current implementation, we do this check after finishing register dependency of MI (line 1040) and when we know that the DestRef of copy is potentially sinkable to a single successor (line 978). I think in practically doing this only when we need to do must be cheaper than doing this in advance for all live-ins of successors independently of an MI. 

The worst case I can think of is that when a BB contain many redundant sinkable Copies  writing to the same DestReg. For that, we might be able to cache the result for already visited Regs for a BB. Then, in worst case, we will perform this loop at most the number of registers of the target for a BB. Please let me know your thought. 


https://reviews.llvm.org/D41463