From 65fef30952bb92acec7ed36f7f431d93f7ce88b3 Mon Sep 17 00:00:00 2001
From: Nicolas Geoffray <ngeoffray@google.com>
Date: Wed, 4 May 2016 14:00:12 +0100
Subject: [PATCH] Relax the DCHECK in load store elimination.

The DCHECK was too strong, as we could come from a field or array
get on null, instead of null directly.

bug:27831001

Change-Id: I24b6a0b3c92467aff81aaeaa22402743e9014d97
---
 compiler/optimizing/load_store_elimination.cc | 15 +++-----
 test/586-checker-null-array-get/src/Main.java | 49 ++++++++++++++++++++++++---
 2 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/compiler/optimizing/load_store_elimination.cc b/compiler/optimizing/load_store_elimination.cc
index 2de41580b..8a75a90cf 100644
--- a/compiler/optimizing/load_store_elimination.cc
+++ b/compiler/optimizing/load_store_elimination.cc
@@ -733,19 +733,14 @@ class LSEVisitor : public HGraphVisitor {
       if (Primitive::PrimitiveKind(heap_value->GetType())
               != Primitive::PrimitiveKind(instruction->GetType())) {
         // The only situation where the same heap location has different type is when
-        // we do an array get from a null constant. In order to stay properly typed
-        // we do not merge the array gets.
+        // we do an array get on an instruction that originates from the null constant
+        // (the null could be behind a field access, an array access, a null check or
+        // a bound type).
+        // In order to stay properly typed on primitive types, we do not eliminate
+        // the array gets.
         if (kIsDebugBuild) {
           DCHECK(heap_value->IsArrayGet()) << heap_value->DebugName();
           DCHECK(instruction->IsArrayGet()) << instruction->DebugName();
-          HInstruction* array = instruction->AsArrayGet()->GetArray();
-          DCHECK(array->IsNullCheck()) << array->DebugName();
-          HInstruction* input = HuntForOriginalReference(array->InputAt(0));
-          DCHECK(input->IsNullConstant()) << input->DebugName();
-          array = heap_value->AsArrayGet()->GetArray();
-          DCHECK(array->IsNullCheck()) << array->DebugName();
-          input = HuntForOriginalReference(array->InputAt(0));
-          DCHECK(input->IsNullConstant()) << input->DebugName();
         }
         return;
       }
diff --git a/test/586-checker-null-array-get/src/Main.java b/test/586-checker-null-array-get/src/Main.java
index 332cfb0a5..e0782bc84 100644
--- a/test/586-checker-null-array-get/src/Main.java
+++ b/test/586-checker-null-array-get/src/Main.java
@@ -14,10 +14,20 @@
  * limitations under the License.
  */
 
+class Test1 {
+  int[] iarr;
+}
+
+class Test2 {
+  float[] farr;
+}
+
 public class Main {
   public static Object[] getObjectArray() { return null; }
   public static long[] getLongArray() { return null; }
   public static Object getNull() { return null; }
+  public static Test1 getNullTest1() { return null; }
+  public static Test2 getNullTest2() { return null; }
 
   public static void main(String[] args) {
     try {
@@ -26,13 +36,25 @@ public class Main {
     } catch (NullPointerException e) {
       // Expected.
     }
+    try {
+      bar();
+      throw new Error("Expected NullPointerException");
+    } catch (NullPointerException e) {
+      // Expected.
+    }
+    try {
+      test1();
+      throw new Error("Expected NullPointerException");
+    } catch (NullPointerException e) {
+      // Expected.
+    }
   }
 
   /// CHECK-START: void Main.foo() load_store_elimination (after)
-  /// CHECK-DAG: <<Null:l\d+>>  NullConstant
-  /// CHECK-DAG: <<Check:l\d+>> NullCheck [<<Null>>]
-  /// CHECK-DAG: <<Get1:j\d+>>  ArrayGet [<<Check>>,{{i\d+}}]
-  /// CHECK-DAG: <<Get2:l\d+>>  ArrayGet [<<Check>>,{{i\d+}}]
+  /// CHECK-DAG: <<Null:l\d+>>   NullConstant
+  /// CHECK-DAG: <<Check:l\d+>>  NullCheck [<<Null>>]
+  /// CHECK-DAG: <<Get1:j\d+>>   ArrayGet [<<Check>>,{{i\d+}}]
+  /// CHECK-DAG: <<Get2:l\d+>>   ArrayGet [<<Check>>,{{i\d+}}]
   public static void foo() {
     longField = getLongArray()[0];
     objectField = getObjectArray()[0];
@@ -56,7 +78,7 @@ public class Main {
     // elimination pass to add a HDeoptimize. Not having the bounds check helped
     // the load store elimination think it could merge two ArrayGet with different
     // types.
-    String[] array = ((String[])getNull());
+    String[] array = (String[])getNull();
     objectField = array[0];
     objectField = array[1];
     objectField = array[2];
@@ -68,6 +90,23 @@ public class Main {
     longField = longArray[3];
   }
 
+  /// CHECK-START: float Main.test1() load_store_elimination (after)
+  /// CHECK-DAG: <<Null:l\d+>>       NullConstant
+  /// CHECK-DAG: <<Check1:l\d+>>     NullCheck [<<Null>>]
+  /// CHECK-DAG: <<FieldGet1:l\d+>>  InstanceFieldGet [<<Check1>>] field_name:Test1.iarr
+  /// CHECK-DAG: <<Check2:l\d+>>     NullCheck [<<FieldGet1>>]
+  /// CHECK-DAG: <<ArrayGet1:i\d+>>  ArrayGet [<<Check2>>,{{i\d+}}]
+  /// CHECK-DAG: <<ArrayGet2:f\d+>>  ArrayGet [<<Check2>>,{{i\d+}}]
+  /// CHECK-DAG:                     Return [<<ArrayGet2>>]
+  public static float test1() {
+    Test1 test1 = getNullTest1();
+    Test2 test2 = getNullTest2();;
+    int[] iarr = test1.iarr;
+    float[] farr = test2.farr;
+    iarr[0] = iarr[1];
+    return farr[0];
+  }
+
   public static long longField;
   public static Object objectField;
 }
-- 
2.11.0