[PATCH] D131587: [CodeGen] Deduplicate restore blocks in branch relaxation

[Piggy via Phabricator via llvm-commits]

piggynl marked an inline comment as not done.
piggynl added a comment.

Currently `SIInstrinfo::insertIndirectBranch` do something like this:

1. Create a new virtual register `PCReg` and two symbols `OffsetLo` and `OffsetHi`.

2. Before the branch instruction, insert instructions to calculate the branch offset with `PCReg`, `OffsetLo`, and `OffsetHi`.

3. Scavenge a free SGPR, or spill an SGPR to VGPR to make it free.

4. Replace `PCReg` with the free SGPR.

5. Set the values of `OffsetLo` and `OffsetHi`.

Only when the spill happens, a restore block will be used and should be deduplicated.

In D131587#3792180 <https://reviews.llvm.org/D131587#3792180>, @arsenm wrote:

> When would an inserted restore block ever not be identical with the same destination?

The restore block will not be identical when the used SGPR or VGPR is different.



================
Comment at: llvm/lib/CodeGen/BranchRelaxation.cpp:105
+          return ItOld == ItEndOld && ItNew == ItEndNew;
+        if (!ItOld->isIdenticalTo(*ItNew))
+          return false;
----------------
arsenm wrote:
> Does this also need to consider the various flags on the block (e.g. alignment)?
No target is constructing the restore block with flagged instructions at this time. I've added a FIXME here.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1010
+
+    MachineBasicBlock *JumpDest = DeduplicateRestoreBB(RestoreBB);
+    MI.getOperand(1).setMBB(JumpDest);
----------------
arsenm wrote:
> Why is this a callback? Why doesn't the pass just deduplicate to begin with and pass in the new block?
In `TargetInstrinfo::insertIndirectBranch()`, targets first optionally build the restore block, then build the indirect branch points to the restore block. The deduplication is only possible in the middle of the two processes. Both two processes may be heavy (for example `SIInstrinfo::insertIndirectBranch()`). I think splitting `insertIndirectBranch()` into two functions is not a good idea because it will increase the complexity for targets doesn't need any restore block.


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

https://reviews.llvm.org/D131587