[PATCH] D41463: [CodeGen] Add a new pass to sink Copy instructions after RA

[Chad Rosier via Phabricator via llvm-commits]

mcrosier added a comment.

I'd like to suggest that this implementation be included in MachineSink.cpp, but continue to live as a separate pass.  The two passes have the same intent, but must be independent passes due to the previously mentioned constrains;  I'd almost want to refer to this as the PostRAMachineSink pass... The point being I'd like to have one place where I can see what's sunk pre-RA and what's sunk post-RA.



================
Comment at: lib/CodeGen/MachineCopySink.cpp:10
+//
+// This pass sink COPY instructions into a successor close to their use, if
+// unused in the current block. In the CodeGen phase, COPY instructions are
----------------
Perhaps something like:

  // This pass sinks COPY instructions into a successor block, if the COPY is not
  // used in the current block and the COPY is live-in to a single successor
  // (i.e., doesn't require the COPY to be duplicated).  This avoids executing the
  // the copy on paths where their results aren't needed.  This also exposes
  // additional opportunites for dead copy elimination and shrink wrapping.
  //
  // These copies were either not handled by or are inserted after the MachineSink
  // pass.  As an example of the former case, the MachineSink pass cannot sink
  // COPY instructions with allocatable source registers; for AArch64 these type
  // of copy instructions are frequently used to move function parameters (PhyReg)
  // into virtual registers in the entry block.
  //
  // For the machine IR below, this pass will sink %w19 in the entry into its
  // successor (%bb.1) because %w19 is only live-in in %bb.1.
  // %bb.0:
  //   %wzr = SUBSWri %w1, 1
  //   %w19 = COPY %w0
  //   Bcc 11, %bb.2
  // %bb.1:
  //   Live Ins: %w19
  //   BL @fun
  //   %w0 = ADDWrr %w0, %w19
  //   RET %w0
  // %bb.2:
  //   %w0 = COPY %wzr
  //   RET %w0
  // As we sink %w19 (CSR in AArch64) into %bb.1, the shrink-wrapping pass will be
  // able to see %bb.0 as a candidate.



================
Comment at: lib/CodeGen/MachineCopySink.cpp:85
+/// and modifies.
+static void trackRegDefsUses(MachineInstr *MI, BitVector &ModifiedRegs,
+                             BitVector &UsedRegs,
----------------
AFAICT, this is a straight copy from the AArch64LoadStoreOptimizer.cpp pass.  Can we refactor this into AArch64InstrInfo, so we don't duplicate code?


================
Comment at: lib/CodeGen/MachineCopySink.cpp:120
+    if (SI->isLiveIn(Reg)) {
+      if (BB)
+        return nullptr;
----------------
  // If BB is set here, Reg is live-in to at least two sinkable successors, so quit.


================
Comment at: lib/CodeGen/MachineCopySink.cpp:126
+
+  if (!BB)
+    return nullptr;
----------------
  // Reg is not live-in to any sinkable successors.


================
Comment at: lib/CodeGen/MachineCopySink.cpp:130
+  // Check if any register aliased with Reg is live-in in other successors.
+  for (auto *SI : CurBB.successors()) {
+    if (SI == BB)
----------------
(A suggestion that can be ignored if you disagree, but) I think this might be easier to understand if this loop and the loop on line 118 were combine.

Something like:
  MachineBasicBlock *BB = nullptr;
  for (auto *SI : CurBB.successors()) {
    // Try to find a single sinkable successor in which Reg is live-in.
    if (SinkableBBs.count(SI) && SI->isLiveIn(Reg)) {
      if (BB)
        return nullptr;
      BB = SI;
      continue;
    }
    // Check if Reg or any register aliased with Reg is live-in in other successors.
    if (SI->isLiveIn(Reg))
      return nullptr;

    for (const auto LI : SI->liveins())
      if (AliasedRegs.count(LI.PhysReg))
        return nullptr;
  }

Of course this means that you'll have to change SinkableBBs to a Set/SmallSet, which I believe you can iterate over..


================
Comment at: lib/CodeGen/MachineCopySink.cpp:175
+    unsigned SrcReg = MI->getOperand(1).getReg();
+
+    if (ModifiedRegs[DefReg] || ModifiedRegs[SrcReg] || UsedRegs[DefReg]) {
----------------
Perhaps add a comment.

  // Don't sink the COPY if it would violate a register dependency.


https://reviews.llvm.org/D41463