[llvm] r287213 - [ImplicitNullCheck] Fix an edge case where we were hoisting incorrectly

[Sanjoy Das via llvm-commits]

Author: sanjoy
Date: Thu Nov 17 01:29:40 2016
New Revision: 287213

URL: http://llvm.org/viewvc/llvm-project?rev=287213&view=rev
Log:
[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.

Modified:
    llvm/trunk/lib/CodeGen/ImplicitNullChecks.cpp
    llvm/trunk/test/CodeGen/X86/implicit-null-checks.mir

Modified: llvm/trunk/lib/CodeGen/ImplicitNullChecks.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/ImplicitNullChecks.cpp?rev=287213&r1=287212&r2=287213&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/ImplicitNullChecks.cpp (original)
+++ llvm/trunk/lib/CodeGen/ImplicitNullChecks.cpp Thu Nov 17 01:29:40 2016
@@ -268,7 +268,25 @@ bool HazardDetector::isSafeToHoist(Machi
             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))

Modified: llvm/trunk/test/CodeGen/X86/implicit-null-checks.mir
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/implicit-null-checks.mir?rev=287213&r1=287212&r2=287213&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/X86/implicit-null-checks.mir (original)
+++ llvm/trunk/test/CodeGen/X86/implicit-null-checks.mir Thu Nov 17 01:29:40 2016
@@ -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
+
+...