[optimizing] Fix a bug in moving the null check to the user.

When taking the decision to move a null check to the user we did not
verify if the next instruction checks the same object.

Change-Id: I2f4533a4bb18aa4b0b6d5e419f37dcccd60354d2

9 files changed, 81 insertions, 19 deletions

diff --git a/compiler/optimizing/code_generator.cc b/compiler/optimizing/code_generator.cc
index 8ab759d39..b14b69ba3 100644
--- a/compiler/optimizing/code_generator.cc
+++ b/compiler/optimizing/code_generator.cc
@@ -827,7 +827,9 @@ void CodeGenerator::RecordPcInfo(HInstruction* instruction,
 
 bool CodeGenerator::CanMoveNullCheckToUser(HNullCheck* null_check) {
   HInstruction* first_next_not_move = null_check->GetNextDisregardingMoves();
-  return (first_next_not_move != nullptr) && first_next_not_move->CanDoImplicitNullCheck();
+
+  return (first_next_not_move != nullptr)
+      && first_next_not_move->CanDoImplicitNullCheckOn(null_check->InputAt(0));
 }
 
 void CodeGenerator::MaybeRecordImplicitNullCheck(HInstruction* instr) {
@@ -842,7 +844,7 @@ void CodeGenerator::MaybeRecordImplicitNullCheck(HInstruction* instr) {
     return;
   }
 
-  if (!instr->CanDoImplicitNullCheck()) {
+  if (!instr->CanDoImplicitNullCheckOn(instr->InputAt(0))) {
     return;
   }
 
diff --git a/compiler/optimizing/intrinsics_arm.cc b/compiler/optimizing/intrinsics_arm.cc
index 9a6062fed..932192e4f 100644
--- a/compiler/optimizing/intrinsics_arm.cc
+++ b/compiler/optimizing/intrinsics_arm.cc
@@ -863,7 +863,7 @@ void IntrinsicCodeGeneratorARM::VisitStringCompareTo(HInvoke* invoke) {
   LocationSummary* locations = invoke->GetLocations();
 
   // Note that the null check must have been done earlier.
-  DCHECK(!invoke->CanDoImplicitNullCheck());
+  DCHECK(!invoke->CanDoImplicitNullCheckOn(invoke->InputAt(0)));
 
   Register argument = locations->InAt(1).AsRegister<Register>();
   __ cmp(argument, ShifterOperand(0));
diff --git a/compiler/optimizing/intrinsics_arm64.cc b/compiler/optimizing/intrinsics_arm64.cc
index d3a4e6ca1..117d6a427 100644
--- a/compiler/optimizing/intrinsics_arm64.cc
+++ b/compiler/optimizing/intrinsics_arm64.cc
@@ -1007,7 +1007,7 @@ void IntrinsicCodeGeneratorARM64::VisitStringCompareTo(HInvoke* invoke) {
   LocationSummary* locations = invoke->GetLocations();
 
   // Note that the null check must have been done earlier.
-  DCHECK(!invoke->CanDoImplicitNullCheck());
+  DCHECK(!invoke->CanDoImplicitNullCheckOn(invoke->InputAt(0)));
 
   Register argument = WRegisterFrom(locations->InAt(1));
   __ Cmp(argument, 0);
diff --git a/compiler/optimizing/intrinsics_x86.cc b/compiler/optimizing/intrinsics_x86.cc
index 95ab90de2..a8e2cdf1f 100644
--- a/compiler/optimizing/intrinsics_x86.cc
+++ b/compiler/optimizing/intrinsics_x86.cc
@@ -962,7 +962,7 @@ void IntrinsicCodeGeneratorX86::VisitStringCompareTo(HInvoke* invoke) {
   LocationSummary* locations = invoke->GetLocations();
 
   // Note that the null check must have been done earlier.
-  DCHECK(!invoke->CanDoImplicitNullCheck());
+  DCHECK(!invoke->CanDoImplicitNullCheckOn(invoke->InputAt(0)));
 
   Register argument = locations->InAt(1).AsRegister<Register>();
   __ testl(argument, argument);
diff --git a/compiler/optimizing/intrinsics_x86_64.cc b/compiler/optimizing/intrinsics_x86_64.cc
index d9a1c31c7..c718ece01 100644
--- a/compiler/optimizing/intrinsics_x86_64.cc
+++ b/compiler/optimizing/intrinsics_x86_64.cc
@@ -886,7 +886,7 @@ void IntrinsicCodeGeneratorX86_64::VisitStringCompareTo(HInvoke* invoke) {
   LocationSummary* locations = invoke->GetLocations();
 
   // Note that the null check must have been done earlier.
-  DCHECK(!invoke->CanDoImplicitNullCheck());
+  DCHECK(!invoke->CanDoImplicitNullCheckOn(invoke->InputAt(0)));
 
   CpuRegister argument = locations->InAt(1).AsRegister<CpuRegister>();
   __ testl(argument, argument);
diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index 1565f5897..08fcdbbcd 100644
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -1158,7 +1158,10 @@ class HInstruction : public ArenaObject<kArenaAllocMisc> {
     return true;
   }
 
-  virtual bool CanDoImplicitNullCheck() const { return false; }
+  virtual bool CanDoImplicitNullCheckOn(HInstruction* obj) const {
+    UNUSED(obj);
+    return false;
+  }
 
   void SetReferenceTypeInfo(ReferenceTypeInfo reference_type_info) {
     DCHECK_EQ(GetType(), Primitive::kPrimNot);
@@ -2225,7 +2228,8 @@ class HInvokeStaticOrDirect : public HInvoke {
         invoke_type_(invoke_type),
         is_recursive_(is_recursive) {}
 
-  bool CanDoImplicitNullCheck() const OVERRIDE {
+  bool CanDoImplicitNullCheckOn(HInstruction* obj) const OVERRIDE {
+    UNUSED(obj);
     // We access the method via the dex cache so we can't do an implicit null check.
     // TODO: for intrinsics we can generate implicit null checks.
     return false;
@@ -2257,9 +2261,9 @@ class HInvokeVirtual : public HInvoke {
       : HInvoke(arena, number_of_arguments, return_type, dex_pc, dex_method_index),
         vtable_index_(vtable_index) {}
 
-  bool CanDoImplicitNullCheck() const OVERRIDE {
+  bool CanDoImplicitNullCheckOn(HInstruction* obj) const OVERRIDE {
     // TODO: Add implicit null checks in intrinsics.
-    return !GetLocations()->Intrinsified();
+    return (obj == InputAt(0)) && !GetLocations()->Intrinsified();
   }
 
   uint32_t GetVTableIndex() const { return vtable_index_; }
@@ -2283,9 +2287,9 @@ class HInvokeInterface : public HInvoke {
       : HInvoke(arena, number_of_arguments, return_type, dex_pc, dex_method_index),
         imt_index_(imt_index) {}
 
-  bool CanDoImplicitNullCheck() const OVERRIDE {
+  bool CanDoImplicitNullCheckOn(HInstruction* obj) const OVERRIDE {
     // TODO: Add implicit null checks in intrinsics.
-    return !GetLocations()->Intrinsified();
+    return (obj == InputAt(0)) && !GetLocations()->Intrinsified();
   }
 
   uint32_t GetImtIndex() const { return imt_index_; }
@@ -2855,8 +2859,8 @@ class HInstanceFieldGet : public HExpression<1> {
     return GetFieldOffset().SizeValue() == other_get->GetFieldOffset().SizeValue();
   }
 
-  bool CanDoImplicitNullCheck() const OVERRIDE {
-    return GetFieldOffset().Uint32Value() < kPageSize;
+  bool CanDoImplicitNullCheckOn(HInstruction* obj) const OVERRIDE {
+    return (obj == InputAt(0)) && GetFieldOffset().Uint32Value() < kPageSize;
   }
 
   size_t ComputeHashCode() const OVERRIDE {
@@ -2889,8 +2893,8 @@ class HInstanceFieldSet : public HTemplateInstruction<2> {
     SetRawInputAt(1, value);
   }
 
-  bool CanDoImplicitNullCheck() const OVERRIDE {
-    return GetFieldOffset().Uint32Value() < kPageSize;
+  bool CanDoImplicitNullCheckOn(HInstruction* obj) const OVERRIDE {
+    return (obj == InputAt(0)) && GetFieldOffset().Uint32Value() < kPageSize;
   }
 
   const FieldInfo& GetFieldInfo() const { return field_info_; }
@@ -2920,7 +2924,8 @@ class HArrayGet : public HExpression<2> {
     UNUSED(other);
     return true;
   }
-  bool CanDoImplicitNullCheck() const OVERRIDE {
+  bool CanDoImplicitNullCheckOn(HInstruction* obj) const OVERRIDE {
+    UNUSED(obj);
     // TODO: We can be smarter here.
     // Currently, the array access is always preceded by an ArrayLength or a NullCheck
     // which generates the implicit null check. There are cases when these can be removed
@@ -2962,7 +2967,8 @@ class HArraySet : public HTemplateInstruction<3> {
     return needs_type_check_;
   }
 
-  bool CanDoImplicitNullCheck() const OVERRIDE {
+  bool CanDoImplicitNullCheckOn(HInstruction* obj) const OVERRIDE {
+    UNUSED(obj);
     // TODO: Same as for ArrayGet.
     return false;
   }
@@ -3014,7 +3020,9 @@ class HArrayLength : public HExpression<1> {
     UNUSED(other);
     return true;
   }
-  bool CanDoImplicitNullCheck() const OVERRIDE { return true; }
+  bool CanDoImplicitNullCheckOn(HInstruction* obj) const OVERRIDE {
+    return obj == InputAt(0);
+  }
 
   DECLARE_INSTRUCTION(ArrayLength);
 
diff --git a/test/479-regression-implicit-null-check/expected.txt b/test/479-regression-implicit-null-check/expected.txt
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/test/479-regression-implicit-null-check/expected.txt
diff --git a/test/479-regression-implicit-null-check/info.txt b/test/479-regression-implicit-null-check/info.txt
new file mode 100644
index 000000000..0bfca8cba
--- /dev/null
+++ b/test/479-regression-implicit-null-check/info.txt
@@ -0,0 +1,2 @@
+Tests a regression in which we moved the null check to an instruction which
+checked a different object. This lead to valid null checks being elided.
diff --git a/test/479-regression-implicit-null-check/src/Main.java b/test/479-regression-implicit-null-check/src/Main.java
new file mode 100644
index 000000000..6b6f2e4d2
--- /dev/null
+++ b/test/479-regression-implicit-null-check/src/Main.java
@@ -0,0 +1,50 @@
+/*
+* Copyright (C) 2015 The Android Open Source Project
+*
+* Licensed under the Apache License, Version 2.0 (the "License");
+* you may not use this file except in compliance with the License.
+* You may obtain a copy of the License at
+*
+*      http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+
+public class Main {
+  public int x = 0;
+
+  public Main(Main c) {
+    // After inlining the graph will look like:
+    //     NullCheck c
+    //     InstanceFieldGet c
+    //     InstanceFieldSet this 3
+    // The dead code will eliminate the InstanceFieldGet and we'll end up with:
+    //     NullCheck c
+    //     InstanceFieldSet this 3
+    // At codegen, when verifying if we can move the null check to the user,
+    // we should check that we actually have the same user (not only that the
+    // next instruction can do implicit null checks).
+    // In this case we should generate code for the NullCheck since the next
+    // instruction checks a different object.
+    c.willBeInlined();
+    x = 3;
+  }
+
+  private int willBeInlined() {
+    return x;
+  }
+
+  public static void main(String[] args) {
+    try {
+      new Main(null);
+      throw new RuntimeException("Failed to throw NullPointerException");
+    } catch (NullPointerException e) {
+      // expected
+    }
+  }
+}