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

[Piggy via Phabricator via llvm-commits]

piggynl added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchRelaxation.cpp:119
+             "RestoreBB to be deduplicated should not be empty");
+      for (auto &DeduplicatedBB : RestoreBlocks)
+        if (isRestoreBlockIdentical(DeduplicatedBB, RestoreBB))
----------------
It is possible to use SmallSet instead of SmallVector for RestoreBlocks. It will enable this function to find the reusable block in O(log N) time, but It will take a lot of work. We have to implement a comparator for MachineBasicBlock like FunctionComparator. Also, we cannot reuse `MachineInstr::isIdenticalTo()` and `MachineOperand::isIdenticalTo()` because they can only return the equality.

I have no idea how frequently is register spill required for branch relaxation in AMDGPU, but in RISCV, it almost never happens. Also, since RISCV generates a same restore block consisting only `ld s11, 0(sp)` for all jumps to one destination (see D130560), there should be no difference for RISCV to use SmallVector or SmallSet.


================
Comment at: llvm/test/CodeGen/AMDGPU/branch-relax-spill.ll:2036
+
+define amdgpu_kernel void @spill_duplicated_restore_block(i32 addrspace(1)* %arg, i32 %cnd) #0 {
+; CHECK-LABEL: spill_duplicated_restore_block:
----------------
I have no knowledge of AMDGPU and don't know how to construct some branches that can share or cann't share a same restore block, so please kindly let me know how to construct the test properly.

This is the first added function. I copied `@spill()` in this file and simply added another branch after the previous one. The two branches look to share the same restore block `.LBB2_5`.


================
Comment at: llvm/test/CodeGen/AMDGPU/branch-relax-spill.ll:2998
+
+define void @spill_func_duplicated_restore_block(i32 addrspace(1)* %arg) #0 {
+; CHECK-LABEL: spill_func_duplicated_restore_block:
----------------
This is the second added function. I copied `@spill_func()` in this file and simply added another branch after the previous one. The two branches look don't have a same restore block.


================
Comment at: llvm/test/CodeGen/AMDGPU/branch-relax-spill.ll:3405-3415
+; CHECK-NEXT:  .LBB3_1: ; %bb4
+; CHECK-NEXT:    v_writelane_b32 v1, vcc_hi, 8
+; CHECK-NEXT:    s_mov_b32 vcc_hi, vcc_lo
+; CHECK-NEXT:    s_mov_b32 vcc_lo, s101
+; CHECK-NEXT:    s_mov_b32 s101, s100
+; CHECK-NEXT:    s_mov_b32 s100, s99
+; CHECK-NEXT:    s_mov_b32 s99, s98
----------------
and I don't know why this sequence is forming. Is this intended?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131587