[CodeGen] Fix restore blocks' BasicBlock information in branch relaxation

[piggynl]

Summary

In branch relaxation pass, restore blocks are created and placed before the jump destination if indirect branches are required. For example:

        foo
        sd      s11, 0(sp)
        jump    .restore, s11
        bar
        bar
        bar
        j       .dest
.restore:
        ld      s11, 0(sp)
.dest:
        baz

The BasicBlock information of the restore MachineBasicBlock should be identical to the dest MachineBasicBlock.

Depends on D131862, which pre-commits the modified test.

[piggynl]

Rebase and ping for review :)

[piggynl]

Ping for review :)

[arsenm]

llvm/lib/CodeGen/BranchRelaxation.cpp

21

Don’t see why you need this include

[arsenm]

What’s the point of copying the IR block?

[arsenm]

What problem are you trying to solve with this? I was wondering if blocks only indirectly derived from source IR blocks should perhaps not have the IR backlinks (but we do currently copy around these IR references in other block splitting contexts)

[arsenm]

I guess there's nothing wrong with this, although I'm not sure split blocks should propagate the underlying IR block, we do that in other places today. I think you can still remove the include

[piggynl]

Removes unnecessary #include.

[arsenm]

LGTM but I would appreciate a note on why you care about this for future reference

[piggynl]

Because if the generated restore blocks have the incorrect BasicBlock information, like the # %dest_3 for .LBB8_6 and .LBB6_10 before this change, they would be confusing. Although I believe it is also acceptable if they contain no corresponding BasicBlock information, it will be easier to understand if they do, such as the '# %dest 1' with '.LBB8_6' and '# %dest 2' with '.LBB6_10' after this change.

[arsenm]

In D131863#3963263, @piggynl wrote:

Because if the generated restore blocks have the incorrect BasicBlock information, like the # %dest_3 for .LBB8_6 and .LBB6_10 before this change, they would be confusing. Although I believe it is also acceptable if they contain no corresponding BasicBlock information, it will be easier to understand if they do, such as the '# %dest 1' with '.LBB8_6' and '# %dest 2' with '.LBB6_10' after this change.

OK, so it's just a comment change - nonfunctional

[piggynl]

Yes, exactly. Thank you for your time on this patch!

In D131863#3963335, @arsenm wrote:

In D131863#3963263, @piggynl wrote:

Because if the generated restore blocks have the incorrect BasicBlock information, like the # %dest_3 for .LBB8_6 and .LBB6_10 before this change, they would be confusing. Although I believe it is also acceptable if they contain no corresponding BasicBlock information, it will be easier to understand if they do, such as the '# %dest 1' with '.LBB8_6' and '# %dest 2' with '.LBB6_10' after this change.

OK, so it's just a comment change - nonfunctional

Yes, exactly. Thank you for your time on this patch!

[hctim]

Looks like this patch might've caused a breakage for check-llvm, although I've done no work to actually validate this (just check-llvm from about 10-behind-head (i.e. without this patch) was okay, and at head right now is broken).

/usr/local/google/home/mitchp/llvm/llvm/test/CodeGen/RISCV/branch-relaxation.ll:2944:20: error: CHECK-RV64-NEXT:
 expected string not found in input                                                                             
; CHECK-RV64-NEXT: sd ra, 16(sp) # 8-byte Folded Spill                                                          
                   ^                                                                                            
<stdin>:1055:9: note: scanning from here                                                                        
 #NO_APP                                                                                                        
        ^                                                                                                       
<stdin>:1139:2: note: possible intended match here                                                              
 sd t5, 16(sp) # 8-byte Folded Spill                                                                            
 ^                                                                                                              

Input file: <stdin>
Check file: /usr/local/google/home/mitchp/llvm/llvm/test/CodeGen/RISCV/branch-relaxation.ll

-dump-input=help explains the following input dump.

Input was:
<<<<<<
             .
             .
             .
          1050:  .cfi_offset s9, -88 
          1051:  .cfi_offset s10, -96 
          1052:  .cfi_offset s11, -104 
          1053:  #APP 
          1054:  li ra, 1 
          1055:  #NO_APP 
next:2944'0             X error: no match found
          1056:  #APP 
next:2944'0     ~~~~~~
          1060:  li t1, 6                                                                                       
next:2944'0     ~~~~~~~~~~                                                                                      
             .                                                                                                  
             .                                                                                                  
             .
          1134:  #APP 
next:2944'0     ~~~~~~
          1135:  li t6, 31 
next:2944'0     ~~~~~~~~~~~
          1136:  #NO_APP 
next:2944'0     ~~~~~~~~~
          1137:  sd t6, 8(sp) # 8-byte Folded Spill 
next:2944'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          1138:  sext.w t6, t6 
next:2944'0     ~~~~~~~~~~~~~~~
          1139:  sd t5, 16(sp) # 8-byte Folded Spill 
next:2944'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
next:2944'1      ?                                    possible intended match
          1140:  sext.w t5, t5 
next:2944'0     ~~~~~~~~~~~~~~~
          1141:  bne t5, t6, .LBB6_1 
next:2944'0     ~~~~~~~~~~~~~~~~~~~~~
          1142: # %bb.7: # %entry 
next:2944'0     ~~~~~~~~~~~~~~~~~~
          1143:  jump .LBB6_4, t5 
next:2944'0     ~~~~~~~~~~~~~~~~~~
          1144: .LBB6_1: # %cond_2 
next:2944'0     ~~~~~~~~~~~~~~~~~~~
             .
             .
             .
>>>>>>

[hctim]

Hmm, looks like it might've just been a problem on my end (or it's been fixed in-tree and didn't update this phab review). Sorry for the noise!

llvm/lib/CodeGen/BranchRelaxation.cpp

 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/RegisterScavenging.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
class BranchRelaxation : public MachineFunctionPass {
 
   MachineFunction *MF;
   const TargetRegisterInfo *TRI;
   const TargetInstrInfo *TII;
 
   bool relaxBranchInstructions();
   void scanFunction();
 
-  MachineBasicBlock *createNewBlockAfter(MachineBasicBlock &BB);
+  MachineBasicBlock *createNewBlockAfter(MachineBasicBlock &OrigMBB);
+  MachineBasicBlock *createNewBlockAfter(MachineBasicBlock &OrigMBB,
+                                         const BasicBlock *BB);
 
   MachineBasicBlock *splitBlockBeforeInstr(MachineInstr &MI,
                                            MachineBasicBlock *DestBB);
   void adjustBlockOffsets(MachineBasicBlock &Start);
   bool isBlockInRange(const MachineInstr &MI, const MachineBasicBlock &BB) const;
 
   bool fixupConditionalBranch(MachineInstr &MI);
   bool fixupUnconditionalBranch(MachineInstr &MI);
for (auto &MBB :
     // Get the offset and known bits at the end of the layout predecessor.
     // Include the alignment of the current block.
     BlockInfo[Num].Offset = BlockInfo[PrevNum].postOffset(MBB);
 
     PrevNum = Num;
   }
 }
 
-/// Insert a new empty basic block and insert it after \BB
-MachineBasicBlock *BranchRelaxation::createNewBlockAfter(MachineBasicBlock &BB) {
+/// Insert a new empty MachineBasicBlock and insert it after \p OrigMBB
+MachineBasicBlock *
+BranchRelaxation::createNewBlockAfter(MachineBasicBlock &OrigBB) {
+  return createNewBlockAfter(OrigBB, OrigBB.getBasicBlock());
+}
+
+/// Insert a new empty MachineBasicBlock with \p BB as its BasicBlock
+/// and insert it after \p OrigMBB
+MachineBasicBlock *
+BranchRelaxation::createNewBlockAfter(MachineBasicBlock &OrigMBB,
+                                      const BasicBlock *BB) {
   // Create a new MBB for the code after the OrigBB.
-  MachineBasicBlock *NewBB =
-      MF->CreateMachineBasicBlock(BB.getBasicBlock());
-  MF->insert(++BB.getIterator(), NewBB);
+  MachineBasicBlock *NewBB = MF->CreateMachineBasicBlock(BB);
+  MF->insert(++OrigMBB.getIterator(), NewBB);
 
   // Insert an entry into BlockInfo to align it properly with the block numbers.
   BlockInfo.insert(BlockInfo.begin() + NewBB->getNumber(), BasicBlockInfo());
 
   return NewBB;
 }
 
 /// Split the basic block containing MI into two blocks, which are joined by
bool BranchRelaxation::fixupUnconditionalBranch(MachineInstr &MI) {
   }
 
   DebugLoc DL = MI.getDebugLoc();
   MI.eraseFromParent();
 
   // Create the optional restore block and, initially, place it at the end of
   // function. That block will be placed later if it's used; otherwise, it will
   // be erased.
-  MachineBasicBlock *RestoreBB = createNewBlockAfter(MF->back());
+  MachineBasicBlock *RestoreBB = createNewBlockAfter(MF->back(),
+                                                     DestBB->getBasicBlock());
 
   TII->insertIndirectBranch(*BranchBB, *DestBB, *RestoreBB, DL,
                             DestOffset - SrcOffset, RS.get());
 
   BlockInfo[BranchBB->getNumber()].Size = computeBlockSize(*BranchBB);
   adjustBlockOffsets(*MBB);
 
   // If RestoreBB is required, try to place just before DestBB.

llvm/test/CodeGen/RISCV/branch-relaxation.ll

 ; CHECK-RV32-NEXT:  # %bb.11: # %cond_3
 ; CHECK-RV32-NEXT:    sw s11, 0(sp)
 ; CHECK-RV32-NEXT:    jump .LBB6_12, s11
 ; CHECK-RV32-NEXT:  .LBB6_3: # %space
 ; CHECK-RV32-NEXT:    #APP
 ; CHECK-RV32-NEXT:    .zero 1048576
 ; CHECK-RV32-NEXT:    #NO_APP
 ; CHECK-RV32-NEXT:    j .LBB6_4
-; CHECK-RV32-NEXT:  .LBB6_8: # %dest_3
+; CHECK-RV32-NEXT:  .LBB6_8: # %dest_1
 ; CHECK-RV32-NEXT:    lw s11, 0(sp)
 ; CHECK-RV32-NEXT:  .LBB6_4: # %dest_1
 ; CHECK-RV32-NEXT:    #APP
 ; CHECK-RV32-NEXT:    # dest 1
 ; CHECK-RV32-NEXT:    #NO_APP
 ; CHECK-RV32-NEXT:    j .LBB6_5
-; CHECK-RV32-NEXT:  .LBB6_10: # %dest_3
+; CHECK-RV32-NEXT:  .LBB6_10: # %dest_2
 ; CHECK-RV32-NEXT:    lw s11, 0(sp)
 ; CHECK-RV32-NEXT:  .LBB6_5: # %dest_2
 ; CHECK-RV32-NEXT:    #APP
 ; CHECK-RV32-NEXT:    # dest 2
 ; CHECK-RV32-NEXT:    #NO_APP
 ; CHECK-RV32-NEXT:    j .LBB6_6
 ; CHECK-RV32-NEXT:  .LBB6_12: # %dest_3
 ; CHECK-RV32-NEXT:    lw s11, 0(sp)