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

[Sebastian Pop via Phabricator via llvm-commits]

sebpop added inline comments.


================
Comment at: lib/CodeGen/MachineSink.cpp:995
+      continue;
+    for (const auto LI : SI->liveins())
+      if (AliasedRegs.count(LI.PhysReg))
----------------
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.



================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:901
+      // value is discarded. No need to track such case as a def.
+      if (!TRI->isConstantPhysReg(Reg))
+        for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
----------------
Looks good, thanks!


https://reviews.llvm.org/D41463