From 4a8fe09040de2dbb7aa865c590a1bb18bf2e4a30 Mon Sep 17 00:00:00 2001
From: Sanjoy Das <sanjoy@playingwithpointers.com>
Date: Thu, 17 Nov 2016 07:29:40 +0000
Subject: [PATCH] [ImplicitNullCheck] Fix an edge case where we were hoisting
 incorrectly

ImplicitNullCheck keeps track of one instruction that the memory
operation depends on that it also hoists with the memory operation.
When hoisting this dependency, it would sometimes clobber a live-in
value to the basic block we were hoisting the two things out of.  Fix
this by explicitly looking for such dependencies.

I also noticed two redundant checks on `MO.isDef()` in IsMIOperandSafe.
They're redundant since register MachineOperands are either Defs or Uses
-- there is no third kind.  I'll change the checks to asserts in a later
commit.

llvm-svn: 287213
---
 llvm/lib/CodeGen/ImplicitNullChecks.cpp        | 20 ++++++++-
 llvm/test/CodeGen/X86/implicit-null-checks.mir | 58 ++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/llvm/lib/CodeGen/ImplicitNullChecks.cpp b/llvm/lib/CodeGen/ImplicitNullChecks.cpp
index 6c35aaa..31d65e6 100644
--- a/llvm/lib/CodeGen/ImplicitNullChecks.cpp
+++ b/llvm/lib/CodeGen/ImplicitNullChecks.cpp
@@ -268,7 +268,25 @@ bool HazardDetector::isSafeToHoist(MachineInstr *MI,
             return false;
           assert((!MO.isDef() || RegDefs.count(MO.getReg())) &&
                  "All defs must be tracked in RegDefs by now!");
-          return !MO.isDef() || RegDefs.find(MO.getReg())->second == MI;
+
+          if (!MO.isDef()) {
+            // FIXME: This is unnecessary, we should be able to
+            // assert(MO.isDef()) here.
+            return true;
+          }
+
+          for (unsigned Reg : RegUses)
+            if (TRI.regsOverlap(Reg, MO.getReg()))
+              return false; // We found a write-after-read
+
+          for (auto &OtherDef : RegDefs) {
+            unsigned OtherReg = OtherDef.first;
+            MachineInstr *OtherMI = OtherDef.second;
+            if (OtherMI != MI && TRI.regsOverlap(OtherReg, MO.getReg()))
+              return false;
+          }
+
+          return true;
         };
 
         if (!all_of(MI->operands(), IsMIOperandSafe))
diff --git a/llvm/test/CodeGen/X86/implicit-null-checks.mir b/llvm/test/CodeGen/X86/implicit-null-checks.mir
index eb3c0d5..96e964c 100644
--- a/llvm/test/CodeGen/X86/implicit-null-checks.mir
+++ b/llvm/test/CodeGen/X86/implicit-null-checks.mir
@@ -95,6 +95,26 @@
     ret i32 0
   }
 
+  define i32 @dependency_live_in_hazard(i32* %ptr, i32** %ptr2, i32* %ptr3) #0 {
+  entry:
+    %val = load i32*, i32** %ptr2
+    %ptr_is_null = icmp eq i32* %ptr, null
+    br i1 %ptr_is_null, label %is_null, label %not_null, !make.implicit !0
+
+  not_null:                                         ; preds = %entry
+    %addend = load i32, i32* %val
+    %result = load i32, i32* %ptr
+    %result.shr = lshr i32 %result, 4
+    %result.and = and i32 %result.shr, 4095
+    %result.add = add i32 %addend, %result.and
+    ret i32 %result.add
+
+  is_null:                                          ; preds = %entry
+    ret i32 0
+  }
+
+  attributes #0 = { "target-features"="+bmi,+bmi2" }
+
   !0 = !{}
 ...
 ---
@@ -312,3 +332,41 @@ body:             |
     RETQ %eax
 
 ...
+---
+name:            dependency_live_in_hazard
+# CHECK-LABEL: name:            dependency_live_in_hazard
+# CHECK:   bb.0.entry:
+# CHECK-NOT: FAULTING_LOAD_OP
+# CHECK: bb.1.not_null:
+
+# Make sure that the BEXTR32rm instruction below is not used to emit
+# an implicit null check -- hoisting it will require hosting the move
+# to %esi and we cannot do that without clobbering the use of %rsi in
+# the first instruction in bb.1.not_null.
+alignment:       4
+tracksRegLiveness: true
+liveins:
+  - { reg: '%rdi' }
+  - { reg: '%rsi' }
+body:             |
+  bb.0.entry:
+    successors: %bb.2.is_null, %bb.1.not_null
+    liveins: %rdi, %rsi
+
+    TEST64rr %rdi, %rdi, implicit-def %eflags
+    JE_1 %bb.2.is_null, implicit killed %eflags
+
+  bb.1.not_null:
+    liveins: %rdi, %rsi
+
+    %rcx = MOV64rm killed %rsi, 1, _, 0, _ :: (load 8 from %ir.ptr2)
+    %esi = MOV32ri 3076
+    %eax = BEXTR32rm killed %rdi, 1, _, 0, _, killed %esi, implicit-def dead %eflags :: (load 4 from %ir.ptr)
+    %eax = ADD32rm killed %eax, killed %rcx, 1, _, 0, _, implicit-def dead %eflags :: (load 4 from %ir.val)
+    RETQ %eax
+
+  bb.2.is_null:
+    %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags
+    RETQ %eax
+
+...
-- 
2.7.4