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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added a comment.

Thanks Sebastian for the review. 
I added inlined reply.



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


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4620
+      // WZR/XZR are not modified even when used as a destination register.
+      if (Reg != AArch64::WZR && Reg != AArch64::XZR)
+        for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
----------------
sebpop wrote:
> This check for the zero registers seems to be the only difference with the generic TII implementation.
> I am thinking that this may be the case for other targets than aarch64.
> You could avoid duplicating all the code in this function by checking the result of a function like
> TII->canModifyRegister(Reg) or
> TII->isReadOnly(Reg)
> 
I cannot see neither TII->canModifyRegister(Reg) nor TII->isReadOnly(Reg), but instead I use TRI->isConstantPhysReg(Reg) in the generic TII. 


================
Comment at: test/CodeGen/Thumb2/ifcvt-no-branch-predictor.ll:122
+  store i32 %n, i32* %q, align 4
   %0 = load i32, i32* %p, align 4
   br label %if.end
----------------
I simply wanted to make r0 (%n) used in both successors so that we can keep the MOV in the entry block. I believe this is the easiest/right way to keep the original intention of this test with this new pass. 


https://reviews.llvm.org/D41463