[CodeGen] Resolve FIXME: Use findPHICopyInsertPoint to find the right place to copy InlineBR blocks

[AtariDreams]

No description provided.

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: AtariDreams (AtariDreams)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/89556.diff

3 Files Affected:

• (modified) llvm/include/llvm/CodeGen/TailDuplicator.h (+4-3)
• (modified) llvm/lib/CodeGen/TailDuplicator.cpp (+13-17)
• (modified) llvm/test/CodeGen/X86/tail-dup-asm-goto.ll (+10-9)

diff --git a/llvm/include/llvm/CodeGen/TailDuplicator.h b/llvm/include/llvm/CodeGen/TailDuplicator.h
index 8fdce301c0ccb1..1e08a791d53804 100644
--- a/llvm/include/llvm/CodeGen/TailDuplicator.h
+++ b/llvm/include/llvm/CodeGen/TailDuplicator.h
@@ -122,9 +122,10 @@ class TailDuplicator {
                      SmallVectorImpl<MachineBasicBlock *> &TDBBs,
                      SmallVectorImpl<MachineInstr *> &Copies,
                      SmallVectorImpl<MachineBasicBlock *> *CandidatePtr);
-  void appendCopies(MachineBasicBlock *MBB,
-                 SmallVectorImpl<std::pair<Register, RegSubRegPair>> &CopyInfos,
-                 SmallVectorImpl<MachineInstr *> &Copies);
+  void
+  appendCopies(MachineBasicBlock *MBB, MachineBasicBlock *TailBB,
+               SmallVectorImpl<std::pair<Register, RegSubRegPair>> &CopyInfos,
+               SmallVectorImpl<MachineInstr *> &Copies);
 
   void removeDeadBlock(
       MachineBasicBlock *MBB,
diff --git a/llvm/lib/CodeGen/TailDuplicator.cpp b/llvm/lib/CodeGen/TailDuplicator.cpp
index f5dd21cb927012..44a758bdb2abb2 100644
--- a/llvm/lib/CodeGen/TailDuplicator.cpp
+++ b/llvm/lib/CodeGen/TailDuplicator.cpp
@@ -38,6 +38,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
+#include "PHIEliminationUtils.h"
 #include <algorithm>
 #include <cassert>
 #include <iterator>
@@ -652,14 +653,6 @@ bool TailDuplicator::shouldTailDuplicate(bool IsSimple,
     if (PreRegAlloc && MI.isCall())
       return false;
 
-    // TailDuplicator::appendCopies will erroneously place COPYs after
-    // INLINEASM_BR instructions after 4b0aa5724fea, which demonstrates the same
-    // bug that was fixed in f7a53d82c090.
-    // FIXME: Use findPHICopyInsertPoint() to find the correct insertion point
-    //        for the COPY when replacing PHIs.
-    if (MI.getOpcode() == TargetOpcode::INLINEASM_BR)
-      return false;
-
     if (MI.isBundle())
       InstrCount += MI.getBundleSize();
     else if (!MI.isPHI() && !MI.isMetaInstruction())
@@ -913,7 +906,7 @@ bool TailDuplicator::tailDuplicate(bool IsSimple, MachineBasicBlock *TailBB,
         duplicateInstruction(&MI, TailBB, PredBB, LocalVRMap, UsedByPhi);
       }
     }
-    appendCopies(PredBB, CopyInfos, Copies);
+    appendCopies(PredBB, TailBB, CopyInfos, Copies);
 
     NumTailDupAdded += TailBB->size() - 1; // subtract one for removed branch
 
@@ -981,7 +974,7 @@ bool TailDuplicator::tailDuplicate(bool IsSimple, MachineBasicBlock *TailBB,
           duplicateInstruction(MI, TailBB, PrevBB, LocalVRMap, UsedByPhi);
           MI->eraseFromParent();
         }
-        appendCopies(PrevBB, CopyInfos, Copies);
+        appendCopies(PrevBB, TailBB, CopyInfos, Copies);
       } else {
         TII->removeBranch(*PrevBB);
         // No PHIs to worry about, just splice the instructions over.
@@ -1045,7 +1038,7 @@ bool TailDuplicator::tailDuplicate(bool IsSimple, MachineBasicBlock *TailBB,
       // from PredBB.
       processPHI(&MI, TailBB, PredBB, LocalVRMap, CopyInfos, UsedByPhi, false);
     }
-    appendCopies(PredBB, CopyInfos, Copies);
+    appendCopies(PredBB, TailBB, CopyInfos, Copies);
   }
 
   return Changed;
@@ -1053,14 +1046,17 @@ bool TailDuplicator::tailDuplicate(bool IsSimple, MachineBasicBlock *TailBB,
 
 /// At the end of the block \p MBB generate COPY instructions between registers
 /// described by \p CopyInfos. Append resulting instructions to \p Copies.
-void TailDuplicator::appendCopies(MachineBasicBlock *MBB,
-      SmallVectorImpl<std::pair<Register, RegSubRegPair>> &CopyInfos,
-      SmallVectorImpl<MachineInstr*> &Copies) {
-  MachineBasicBlock::iterator Loc = MBB->getFirstTerminator();
+void TailDuplicator::appendCopies(
+    MachineBasicBlock *MBB, MachineBasicBlock *TailBB,
+    SmallVectorImpl<std::pair<Register, RegSubRegPair>> &CopyInfos,
+    SmallVectorImpl<MachineInstr *> &Copies) {
   const MCInstrDesc &CopyD = TII->get(TargetOpcode::COPY);
+
   for (auto &CI : CopyInfos) {
-    auto C = BuildMI(*MBB, Loc, DebugLoc(), CopyD, CI.first)
-                .addReg(CI.second.Reg, 0, CI.second.SubReg);
+    MachineBasicBlock::iterator InsertPos =
+        findPHICopyInsertPoint(MBB, TailBB, CI.second.Reg);
+    auto C = BuildMI(*MBB, InsertPos, DebugLoc(), CopyD, CI.first)
+                 .addReg(CI.second.Reg, 0, CI.second.SubReg);
     Copies.push_back(C);
   }
 }
diff --git a/llvm/test/CodeGen/X86/tail-dup-asm-goto.ll b/llvm/test/CodeGen/X86/tail-dup-asm-goto.ll
index 05fefbe750e515..14831d8b9d2ab7 100644
--- a/llvm/test/CodeGen/X86/tail-dup-asm-goto.ll
+++ b/llvm/test/CodeGen/X86/tail-dup-asm-goto.ll
@@ -20,30 +20,31 @@ define ptr @test1(ptr %arg1, ptr %arg2) {
   ; CHECK-NEXT:   JMP_1 %bb.1
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1.bb100:
-  ; CHECK-NEXT:   successors: %bb.3(0x80000000)
+  ; CHECK-NEXT:   successors: %bb.5(0x80000000), %bb.4(0x00000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   MOV64mi32 [[COPY1]], 1, $noreg, 0, $noreg, 0 :: (store (s64) into %ir.arg1)
-  ; CHECK-NEXT:   JMP_1 %bb.3
+  ; CHECK-NEXT:   INLINEASM_BR &"#$0 $1 $2", 9 /* sideeffect mayload attdialect */, 13 /* imm */, 42, 13 /* imm */, 0, 13 /* imm */, %bb.4, 12 /* clobber */, implicit-def early-clobber $df, 12 /* clobber */, implicit-def early-clobber $fpsw, 12 /* clobber */, implicit-def early-clobber $eflags
+  ; CHECK-NEXT:   [[COPY2:%[0-9]+]]:gr64 = COPY [[MOV64rm]]
+  ; CHECK-NEXT:   JMP_1 %bb.5
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2.bb106:
-  ; CHECK-NEXT:   successors: %bb.3(0x80000000)
+  ; CHECK-NEXT:   successors: %bb.5(0x80000000), %bb.4(0x00000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
   ; CHECK-NEXT:   CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp
   ; CHECK-NEXT:   ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
-  ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT: bb.3.bb110:
-  ; CHECK-NEXT:   successors: %bb.5(0x80000000), %bb.4(0x00000000)
-  ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   [[PHI:%[0-9]+]]:gr64 = PHI [[COPY]], %bb.2, [[MOV64rm]], %bb.1
   ; CHECK-NEXT:   INLINEASM_BR &"#$0 $1 $2", 9 /* sideeffect mayload attdialect */, 13 /* imm */, 42, 13 /* imm */, 0, 13 /* imm */, %bb.4, 12 /* clobber */, implicit-def early-clobber $df, 12 /* clobber */, implicit-def early-clobber $fpsw, 12 /* clobber */, implicit-def early-clobber $eflags
+  ; CHECK-NEXT:   [[COPY3:%[0-9]+]]:gr64 = COPY [[COPY]]
   ; CHECK-NEXT:   JMP_1 %bb.5
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.4.bb17.i.i.i (machine-block-address-taken, inlineasm-br-indirect-target):
   ; CHECK-NEXT:   successors: %bb.5(0x80000000)
   ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   [[PHI:%[0-9]+]]:gr64 = PHI [[COPY2]], %bb.1, [[COPY3]], %bb.2
+  ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.5.kmem_cache_has_cpu_partial.exit:
-  ; CHECK-NEXT:   $rax = COPY [[PHI]]
+  ; CHECK-NEXT:   [[PHI1:%[0-9]+]]:gr64 = PHI [[PHI]], %bb.4, [[COPY2]], %bb.1, [[COPY3]], %bb.2
+  ; CHECK-NEXT:   $rax = COPY [[PHI1]]
   ; CHECK-NEXT:   RET 0, $rax
 bb:
   %i28.i = load ptr, ptr %arg1, align 8

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.