[ImplicitNullCheck] Extend canReorder scope

[skatkov]

Summary

This change allows a re-order of two intructions if their uses
are overlapped.

[skatkov]

This change has been done on top of https://reviews.llvm.org/D29119 in my workspace, so in diff you can see code from that change. While the changes are not overlapped sorry for inconvinience.

[sanjoy]

Hi Serguei,

The code itself LGTM.

However:

Can you please try to write MIR test cases for this? That's typically
a more straightforward way to unit test codegen passes.
implicit-null-check.ll was added before the MIR infrastructure was
added to LLVM. You can get started with MIR at
http://llvm.org/docs/MIRLangRef.html, and may use
https://reviews.llvm.org/rL293126 as a template to write your own
test.

Secondly, phabricator tells me that you've changed the permissions on
the .ll files; please make sure you've fixed them before checkin.

[skatkov]

Sanjoy, please take a look.

[sanjoy]

lgtm, but I believe you'll also have to edit the test case I checked in in https://reviews.llvm.org/rL293126

Let me know if you want me to check this in for you. Alternatively, you can ask someone from our Saint Petersburg office.

[skatkov]

This comment was removed by skatkov.

[skatkov]

I will modify test and upload new version.

[skatkov]

The test has been updated. It becomes positive now due to extension of the scope.

[skatkov]

One more place should be fixed I guess.

lib/CodeGen/ImplicitNullChecks.cpp

350–351

I think this should be fixed as well as check only for defs. Will try to write a test today.

[skatkov]

Asssert fixed. Also one test is added. The test passes with assert fix and fails witout it.

[skatkov]

Just a rebase after isSuitable... patch is landed.

lib/CodeGen/ImplicitNullChecks.cpp

for (auto MOA : A->operands()) {
 
     unsigned RegA = MOA.getReg();
     for (auto MOB : B->operands()) {
       if (!(MOB.isReg() && MOB.getReg()))
         continue;
 
       unsigned RegB = MOB.getReg();
 
-      if (TRI->regsOverlap(RegA, RegB))
+      if (TRI->regsOverlap(RegA, RegB) && (MOA.isDef() || MOB.isDef()))
         return false;
     }
   }
 
   return true;
 }
 
 bool ImplicitNullChecks::runOnMachineFunction(MachineFunction &MF) {
for (auto &DependenceMO : DependenceMI->operands()) {
     // case the pointer loaded from is in the null page, the load will not
     // semantically execute, and affect machine state.  That is, if the load
     // was loading into %rax and it faults, the value of %rax should stay the
     // same as it would have been had the load not have executed and we'd have
     // branched to NullSucc directly.
     if (AnyAliasLiveIn(TRI, NullSucc, DependenceMO.getReg()))
       return false;
 
     // The Dependency can't be re-defining the base register -- then we won't
     // get the memory operation on the address we want. So bail out.
     if (TRI->regsOverlap(DependenceMO.getReg(), PointerReg))
       return false;
   }
 
   auto DepDepResult =
       computeDependence(DependenceMI, {InstsSeenSoFar.begin(), DependenceItr});
 
   if (!DepDepResult.CanReorder || DepDepResult.PotentialDependence)

test/CodeGen/X86/implicit-null-check.ll

 
  ret_100:
   ret i32 100
 
  ret_200:
   ret i32 200
 }
 
+define i32 @imp_null_check_gep_load_with_use_dep(i32* %x, i32 %a) {
+; CHECK-LABEL: imp_null_check_gep_load_with_use_dep:
+; CHECK: [[BB0_imp_null_check_gep_load_with_use_dep:L[^:]+]]:
+; CHECK: movl (%rdi), %eax
+; CHECK: addl %edi, %esi
+; CHECK: leal 4(%rax,%rsi), %eax
+; CHECK: retq
+; CHECK: [[BB1_imp_null_check_gep_load_with_use_dep:LBB5_[0-9]+]]:
+; CHECK: movl $42, %eax
+; CHECK: retq
+
+ entry:
+  %c = icmp eq i32* %x, null
+  br i1 %c, label %is_null, label %not_null, !make.implicit !0
+
+ is_null:
+  ret i32 42
+
+ not_null:
+  %x.loc = getelementptr i32, i32* %x, i32 1
+  %y = ptrtoint i32* %x.loc to i32
+  %b = add i32 %a, %y
+  %t = load i32, i32* %x
+  %z = add i32 %t, %b
+  ret i32 %z
+}
+
 !0 = !{}
 
 ; CHECK-LABEL: __LLVM_FaultMaps:
 
 ; Version:
 ; CHECK-NEXT: .byte 1
 
 ; Reserved x2
 ; CHECK-NEXT: .byte 0
 ; CHECK-NEXT: .short 0
 
 ; # functions:
-; CHECK-NEXT: .long 5
+; CHECK-NEXT: .long 6
 
 ; FunctionAddr:
 ; CHECK-NEXT: .quad _imp_null_check_add_result
 ; NumFaultingPCs
 ; CHECK-NEXT: .long 1
 ; Reserved:
 ; CHECK-NEXT: .long 0
 ; Fault[0].Type:

 ; Fault[0].Type:
 ; CHECK-NEXT: .long 1
 ; Fault[0].FaultOffset:
 ; CHECK-NEXT: .long [[BB0_imp_null_check_gep_load]]-_imp_null_check_gep_load
 ; Fault[0].HandlerOffset:
 ; CHECK-NEXT: .long [[BB1_imp_null_check_gep_load]]-_imp_null_check_gep_load
 
 ; FunctionAddr:
+; CHECK-NEXT: .quad _imp_null_check_gep_load_with_use_dep
+; NumFaultingPCs
+; CHECK-NEXT: .long 1
+; Reserved:
+; CHECK-NEXT: .long 0
+; Fault[0].Type:
+; CHECK-NEXT: .long 1
+; Fault[0].FaultOffset:
+; CHECK-NEXT: .long [[BB0_imp_null_check_gep_load_with_use_dep]]-_imp_null_check_gep_load_with_use_dep
+; Fault[0].HandlerOffset:
+; CHECK-NEXT: .long [[BB1_imp_null_check_gep_load_with_use_dep]]-_imp_null_check_gep_load_with_use_dep
+
+; FunctionAddr:
 ; CHECK-NEXT: .quad _imp_null_check_hoist_over_unrelated_load
 ; NumFaultingPCs
 ; CHECK-NEXT: .long 1
 ; Reserved:
 ; CHECK-NEXT: .long 0
 ; Fault[0].Type:
 ; CHECK-NEXT: .long 1
 ; Fault[0].FaultOffset:

 ; CHECK-NEXT: .long   1
 ; Fault[0].FaultOffset:
 ; CHECK-NEXT: .long   [[BB0_imp_null_check_via_mem_comparision]]-_imp_null_check_via_mem_comparision
 ; Fault[0].HandlerOffset:
 ; CHECK-NEXT: .long   [[BB1_imp_null_check_via_mem_comparision]]-_imp_null_check_via_mem_comparision
 
 ; OBJDUMP: FaultMap table:
 ; OBJDUMP-NEXT: Version: 0x1
-; OBJDUMP-NEXT: NumFunctions: 5
+; OBJDUMP-NEXT: NumFunctions: 6
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
 ; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 5
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
 ; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 7
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
+; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 9
+; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
 ; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 7
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
 ; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 3