From dd9f94c2928746173fd013cbf5aaca37d5c5be17 Mon Sep 17 00:00:00 2001
From: Puyan Lotfi <puyan@puyan.org>
Date: Sat, 30 Nov 2019 19:00:10 -0500
Subject: [PATCH] [llvm] Fixing MIRVRegNamerUtils to properly handle 2+
 MachineBasicBlocks.

An interplay of code from D70210, along with code from the
Value-Numbering-esque hash-based namer from D70210, as well as some
crusty code from the original MIR-Canon code lead to multiple causes of
failure when canonicalizing or renaming vregs for MIR with multiple
basic blocks. This patch fixes those issues while deleting some no
longer needed code and adding a nice diamond test case to boot.

Differential Revision: https://reviews.llvm.org/D70478
---
 lib/CodeGen/MIRCanonicalizerPass.cpp       | 34 ++----------
 lib/CodeGen/MIRNamerPass.cpp               |  3 +-
 lib/CodeGen/MIRVRegNamerUtils.h            |  2 +-
 test/CodeGen/MIR/X86/mir-canon-hash-bb.mir | 61 ++++++++++++++++++++++
 4 files changed, 69 insertions(+), 31 deletions(-)
 create mode 100644 test/CodeGen/MIR/X86/mir-canon-hash-bb.mir

diff --git a/lib/CodeGen/MIRCanonicalizerPass.cpp b/lib/CodeGen/MIRCanonicalizerPass.cpp
index 7a57cd6890d..5ef907b8831 100644
--- a/lib/CodeGen/MIRCanonicalizerPass.cpp
+++ b/lib/CodeGen/MIRCanonicalizerPass.cpp
@@ -49,10 +49,6 @@ static cl::opt<unsigned>
                                cl::value_desc("N"),
                                cl::desc("Function number to canonicalize."));
 
-static cl::opt<unsigned> CanonicalizeBasicBlockNumber(
-    "canon-nth-basicblock", cl::Hidden, cl::init(~0u), cl::value_desc("N"),
-    cl::desc("BasicBlock number to canonicalize."));
-
 namespace {
 
 class MIRCanonicalizer : public MachineFunctionPass {
@@ -374,24 +370,7 @@ static bool doDefKillClear(MachineBasicBlock *MBB) {
 }
 
 static bool runOnBasicBlock(MachineBasicBlock *MBB,
-                            std::vector<StringRef> &bbNames,
-                            unsigned &basicBlockNum, VRegRenamer &Renamer) {
-
-  if (CanonicalizeBasicBlockNumber != ~0U) {
-    if (CanonicalizeBasicBlockNumber != basicBlockNum++)
-      return false;
-    LLVM_DEBUG(dbgs() << "\n Canonicalizing BasicBlock " << MBB->getName()
-                      << "\n";);
-  }
-
-  if (llvm::find(bbNames, MBB->getName()) != bbNames.end()) {
-    LLVM_DEBUG({
-      dbgs() << "Found potentially duplicate BasicBlocks: " << MBB->getName()
-             << "\n";
-    });
-    return false;
-  }
-
+                            unsigned BasicBlockNum, VRegRenamer &Renamer) {
   LLVM_DEBUG({
     dbgs() << "\n\n  NEW BASIC BLOCK: " << MBB->getName() << "  \n\n";
     dbgs() << "\n\n================================================\n\n";
@@ -399,7 +378,6 @@ static bool runOnBasicBlock(MachineBasicBlock *MBB,
 
   bool Changed = false;
 
-  bbNames.push_back(MBB->getName());
   LLVM_DEBUG(dbgs() << "\n\n NEW BASIC BLOCK: " << MBB->getName() << "\n\n";);
 
   LLVM_DEBUG(dbgs() << "MBB Before Canonical Copy Propagation:\n";
@@ -412,8 +390,10 @@ static bool runOnBasicBlock(MachineBasicBlock *MBB,
   Changed |= rescheduleCanonically(IdempotentInstCount, MBB);
   LLVM_DEBUG(dbgs() << "MBB After Scheduling:\n"; MBB->dump(););
 
-  Changed |= Renamer.renameVRegs(MBB);
+  Changed |= Renamer.renameVRegs(MBB, BasicBlockNum);
 
+  // TODO: Consider dropping this. Dropping kill defs is probably not
+  // semantically sound.
   Changed |= doDefKillClear(MBB);
 
   LLVM_DEBUG(dbgs() << "Updated MachineBasicBlock:\n"; MBB->dump();
@@ -445,16 +425,12 @@ bool MIRCanonicalizer::runOnMachineFunction(MachineFunction &MF) {
            : RPOList) { dbgs() << MBB->getName() << "\n"; } dbgs()
       << "\n\n================================================\n\n";);
 
-  std::vector<StringRef> BBNames;
-
   unsigned BBNum = 0;
-
   bool Changed = false;
-
   MachineRegisterInfo &MRI = MF.getRegInfo();
   VRegRenamer Renamer(MRI);
   for (auto MBB : RPOList)
-    Changed |= runOnBasicBlock(MBB, BBNames, BBNum, Renamer);
+    Changed |= runOnBasicBlock(MBB, BBNum++, Renamer);
 
   return Changed;
 }
diff --git a/lib/CodeGen/MIRNamerPass.cpp b/lib/CodeGen/MIRNamerPass.cpp
index 62d0f2e52c7..9f61dd9ef24 100644
--- a/lib/CodeGen/MIRNamerPass.cpp
+++ b/lib/CodeGen/MIRNamerPass.cpp
@@ -57,9 +57,10 @@ public:
 
     VRegRenamer Renamer(MF.getRegInfo());
 
+    unsigned BBIndex = 0;
     ReversePostOrderTraversal<MachineBasicBlock *> RPOT(&*MF.begin());
     for (auto &MBB : RPOT)
-      Changed |= Renamer.renameVRegs(MBB);
+      Changed |= Renamer.renameVRegs(MBB, BBIndex++);
 
     return Changed;
   }
diff --git a/lib/CodeGen/MIRVRegNamerUtils.h b/lib/CodeGen/MIRVRegNamerUtils.h
index ebe309757f2..8e76bfa2bbd 100644
--- a/lib/CodeGen/MIRVRegNamerUtils.h
+++ b/lib/CodeGen/MIRVRegNamerUtils.h
@@ -84,7 +84,7 @@ public:
 
   /// Same as the above, but sets a BBNum depending on BB traversal that
   /// will be used as prefix for the vreg names.
-  bool renameVRegs(MachineBasicBlock *MBB, unsigned BBNum = 0);
+  bool renameVRegs(MachineBasicBlock *MBB, unsigned BBNum);
 
   unsigned getCurrentBBNumber() const { return CurrentBBNumber; }
 };
diff --git a/test/CodeGen/MIR/X86/mir-canon-hash-bb.mir b/test/CodeGen/MIR/X86/mir-canon-hash-bb.mir
new file mode 100644
index 00000000000..94c69f1be36
--- /dev/null
+++ b/test/CodeGen/MIR/X86/mir-canon-hash-bb.mir
@@ -0,0 +1,61 @@
+# RUN: llc  -run-pass mir-namer -x mir -verify-machineinstrs %s -o - | FileCheck %s
+# RUN: llc  -run-pass mir-canonicalizer -x mir -verify-machineinstrs %s -o - | FileCheck %s
+--- |
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  define i32 @_Z1fi(i32 %arg) {
+    %tmp = alloca i32, align 4
+    %tmp1 = alloca i32, align 4
+    ret i32 %arg
+  }
+
+...
+---
+name:            _Z1fi
+registers:
+  - { id: 0, class: _, preferred-register: '' }
+  - { id: 1, class: _, preferred-register: '' }
+  - { id: 2, class: _, preferred-register: '' }
+  - { id: 3, class: _, preferred-register: '' }
+  - { id: 4, class: _, preferred-register: '' }
+  - { id: 5, class: _, preferred-register: '' }
+  - { id: 6, class: _, preferred-register: '' }
+  - { id: 7, class: _, preferred-register: '' }
+  - { id: 8, class: _, preferred-register: '' }
+stack:
+  - { id: 0, name: tmp, type: default, offset: 0, size: 4, alignment: 4  }
+  - { id: 1, name: tmp1, type: default, offset: 0, size: 4, alignment: 4 }
+body:             |
+  bb.0:
+    %tmp0:_(s32) = COPY $edi
+    %tmp1:_(s32) = G_CONSTANT i32 0
+    %tmp5:_(p0) = G_FRAME_INDEX %stack.0.tmp
+    %tmp6:_(p0) = G_FRAME_INDEX %stack.1.tmp1
+    G_STORE %tmp0(s32), %tmp5(p0) :: (store 4 into %ir.tmp)
+    %tmp7:_(s32) = G_LOAD %tmp5(p0) :: (load 4 from %ir.tmp)
+    %tmp8:_(s1) = G_ICMP intpred(ne), %tmp7(s32), %tmp1
+    G_BRCOND %tmp8(s1), %bb.1
+    G_BR %bb.2
+
+  ; CHECK: bb.1:
+  ; CHECK: %bb2_{{[0-9]+}}__1:_(s32) = G_CONSTANT
+  bb.1:
+    %tmp4:_(s32) = G_CONSTANT i32 1
+    G_STORE %tmp4(s32), %tmp6(p0) :: (store 4 into %ir.tmp1)
+    G_BR %bb.3
+
+
+  ; CHECK: bb.2:
+  ; CHECK: %bb1_{{[0-9]+}}__1:_(s32) = G_CONSTANT
+  bb.2:
+    %tmp3:_(s32) = G_CONSTANT i32 2
+    G_STORE %tmp3(s32), %tmp6(p0) :: (store 4 into %ir.tmp1)
+
+  ; CHECK: bb.3:
+  ; CHECK: %bb3_{{[0-9]+}}__1:_(s32) =  G_LOAD
+  bb.3:
+    %tmp9:_(s32) = G_LOAD %tmp6(p0) :: (load 4 from %ir.tmp1)
+    $eax = COPY %tmp9(s32)
+    RET 0, implicit $eax
+
+...