Revert "Remove the no-longer-needed F/I and D/J alias."

This reverts commit 2f52064dcfe5ebce5a998d30766ca079a366c920.

Reason:

Arrays.sort() returns wrong result on double[] and this CL is the most likely suspect. Rolling back to buy some time for careful analysis and debugging.

Change-Id: I58223c42e95c2287520eef863fbcb738b0736d4d

diff --git a/compiler/optimizing/licm_test.cc b/compiler/optimizing/licm_test.cc
index 2a62643..d446539 100644 (file)
--- a/compiler/optimizing/licm_test.cc
+++ b/compiler/optimizing/licm_test.cc
@@ -169,11 +169,13 @@ TEST_F(LICMTest, ArrayHoisting) {
   BuildLoop();
 
   // Populate the loop with instructions: set/get array with different types.
+  // ArrayGet is typed as kPrimByte and ArraySet given a float value in order to
+  // avoid SsaBuilder's typing of ambiguous array operations from reference type info.
   HInstruction* get_array = new (&allocator_) HArrayGet(
-      parameter_, int_constant_, Primitive::kPrimInt, 0);
+      parameter_, int_constant_, Primitive::kPrimByte, 0);
   loop_body_->InsertInstructionBefore(get_array, loop_body_->GetLastInstruction());
   HInstruction* set_array = new (&allocator_) HArraySet(
-      parameter_, int_constant_, float_constant_, Primitive::kPrimFloat, 0);
+      parameter_, int_constant_, float_constant_, Primitive::kPrimShort, 0);
   loop_body_->InsertInstructionBefore(set_array, loop_body_->GetLastInstruction());
 
   EXPECT_EQ(get_array->GetBlock(), loop_body_);
@@ -187,11 +189,13 @@ TEST_F(LICMTest, NoArrayHoisting) {
   BuildLoop();
 
   // Populate the loop with instructions: set/get array with same types.
+  // ArrayGet is typed as kPrimByte and ArraySet given a float value in order to
+  // avoid SsaBuilder's typing of ambiguous array operations from reference type info.
   HInstruction* get_array = new (&allocator_) HArrayGet(
-      parameter_, int_constant_, Primitive::kPrimFloat, 0);
+      parameter_, int_constant_, Primitive::kPrimByte, 0);
   loop_body_->InsertInstructionBefore(get_array, loop_body_->GetLastInstruction());
   HInstruction* set_array = new (&allocator_) HArraySet(
-      parameter_, get_array, float_constant_, Primitive::kPrimFloat, 0);
+      parameter_, get_array, float_constant_, Primitive::kPrimByte, 0);
   loop_body_->InsertInstructionBefore(set_array, loop_body_->GetLastInstruction());
 
   EXPECT_EQ(get_array->GetBlock(), loop_body_);

diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index 6f3e536..dc5a8fa 100644 (file)
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -1551,21 +1551,21 @@ class SideEffects : public ValueObject {
   static SideEffects FieldWriteOfType(Primitive::Type type, bool is_volatile) {
     return is_volatile
         ? AllWritesAndReads()
-        : SideEffects(TypeFlag(type, kFieldWriteOffset));
+        : SideEffects(TypeFlagWithAlias(type, kFieldWriteOffset));
   }
 
   static SideEffects ArrayWriteOfType(Primitive::Type type) {
-    return SideEffects(TypeFlag(type, kArrayWriteOffset));
+    return SideEffects(TypeFlagWithAlias(type, kArrayWriteOffset));
   }
 
   static SideEffects FieldReadOfType(Primitive::Type type, bool is_volatile) {
     return is_volatile
         ? AllWritesAndReads()
-        : SideEffects(TypeFlag(type, kFieldReadOffset));
+        : SideEffects(TypeFlagWithAlias(type, kFieldReadOffset));
   }
 
   static SideEffects ArrayReadOfType(Primitive::Type type) {
-    return SideEffects(TypeFlag(type, kArrayReadOffset));
+    return SideEffects(TypeFlagWithAlias(type, kArrayReadOffset));
   }
 
   static SideEffects CanTriggerGC() {
@@ -1692,6 +1692,23 @@ class SideEffects : public ValueObject {
   static constexpr uint64_t kAllReads =
       ((1ULL << (kLastBitForReads + 1 - kFieldReadOffset)) - 1) << kFieldReadOffset;
 
+  // Work around the fact that HIR aliases I/F and J/D.
+  // TODO: remove this interceptor once HIR types are clean
+  static uint64_t TypeFlagWithAlias(Primitive::Type type, int offset) {
+    switch (type) {
+      case Primitive::kPrimInt:
+      case Primitive::kPrimFloat:
+        return TypeFlag(Primitive::kPrimInt, offset) |
+               TypeFlag(Primitive::kPrimFloat, offset);
+      case Primitive::kPrimLong:
+      case Primitive::kPrimDouble:
+        return TypeFlag(Primitive::kPrimLong, offset) |
+               TypeFlag(Primitive::kPrimDouble, offset);
+      default:
+        return TypeFlag(type, offset);
+    }
+  }
+
   // Translates type to bit flag.
   static uint64_t TypeFlag(Primitive::Type type, int offset) {
     CHECK_NE(type, Primitive::kPrimVoid);
@@ -5179,8 +5196,10 @@ class HArraySet : public HTemplateInstruction<3> {
             uint32_t dex_pc,
             SideEffects additional_side_effects = SideEffects::None())
       : HTemplateInstruction(
-          SideEffectsForArchRuntimeCalls(value->GetType()).Union(additional_side_effects),
-          dex_pc) {
+            SideEffects::ArrayWriteOfType(expected_component_type).Union(
+                SideEffectsForArchRuntimeCalls(value->GetType())).Union(
+                    additional_side_effects),
+            dex_pc) {
     SetPackedField<ExpectedComponentTypeField>(expected_component_type);
     SetPackedFlag<kFlagNeedsTypeCheck>(value->GetType() == Primitive::kPrimNot);
     SetPackedFlag<kFlagValueCanBeNull>(true);
@@ -5188,8 +5207,6 @@ class HArraySet : public HTemplateInstruction<3> {
     SetRawInputAt(0, array);
     SetRawInputAt(1, index);
     SetRawInputAt(2, value);
-    // We can now call component type logic to set correct type-based side effects.
-    AddSideEffects(SideEffects::ArrayWriteOfType(GetComponentType()));
   }
 
   bool NeedsEnvironment() const OVERRIDE {

diff --git a/compiler/optimizing/side_effects_test.cc b/compiler/optimizing/side_effects_test.cc
index b01bc1c..9bbc354 100644 (file)
--- a/compiler/optimizing/side_effects_test.cc
+++ b/compiler/optimizing/side_effects_test.cc
@@ -148,19 +148,19 @@ TEST(SideEffectsTest, VolatileDependences) {
   EXPECT_FALSE(any_write.MayDependOn(volatile_read));
 }
 
-TEST(SideEffectsTest, SameWidthTypesNoAlias) {
+TEST(SideEffectsTest, SameWidthTypes) {
   // Type I/F.
-  testNoWriteAndReadDependence(
+  testWriteAndReadDependence(
       SideEffects::FieldWriteOfType(Primitive::kPrimInt, /* is_volatile */ false),
       SideEffects::FieldReadOfType(Primitive::kPrimFloat, /* is_volatile */ false));
-  testNoWriteAndReadDependence(
+  testWriteAndReadDependence(
       SideEffects::ArrayWriteOfType(Primitive::kPrimInt),
       SideEffects::ArrayReadOfType(Primitive::kPrimFloat));
   // Type L/D.
-  testNoWriteAndReadDependence(
+  testWriteAndReadDependence(
       SideEffects::FieldWriteOfType(Primitive::kPrimLong, /* is_volatile */ false),
       SideEffects::FieldReadOfType(Primitive::kPrimDouble, /* is_volatile */ false));
-  testNoWriteAndReadDependence(
+  testWriteAndReadDependence(
       SideEffects::ArrayWriteOfType(Primitive::kPrimLong),
       SideEffects::ArrayReadOfType(Primitive::kPrimDouble));
 }
@@ -216,32 +216,14 @@ TEST(SideEffectsTest, BitStrings) {
       "||||||L|",
       SideEffects::FieldWriteOfType(Primitive::kPrimNot, false).ToString().c_str());
   EXPECT_STREQ(
-      "||DFJISCBZL|DFJISCBZL||DFJISCBZL|DFJISCBZL|",
-      SideEffects::FieldWriteOfType(Primitive::kPrimNot, true).ToString().c_str());
-  EXPECT_STREQ(
       "|||||Z||",
       SideEffects::ArrayWriteOfType(Primitive::kPrimBoolean).ToString().c_str());
   EXPECT_STREQ(
-      "|||||C||",
-      SideEffects::ArrayWriteOfType(Primitive::kPrimChar).ToString().c_str());
-  EXPECT_STREQ(
-      "|||||S||",
-      SideEffects::ArrayWriteOfType(Primitive::kPrimShort).ToString().c_str());
-  EXPECT_STREQ(
       "|||B||||",
       SideEffects::FieldReadOfType(Primitive::kPrimByte, false).ToString().c_str());
   EXPECT_STREQ(
-      "||D|||||",
+      "||DJ|||||",  // note: DJ alias
       SideEffects::ArrayReadOfType(Primitive::kPrimDouble).ToString().c_str());
-  EXPECT_STREQ(
-      "||J|||||",
-      SideEffects::ArrayReadOfType(Primitive::kPrimLong).ToString().c_str());
-  EXPECT_STREQ(
-      "||F|||||",
-      SideEffects::ArrayReadOfType(Primitive::kPrimFloat).ToString().c_str());
-  EXPECT_STREQ(
-      "||I|||||",
-      SideEffects::ArrayReadOfType(Primitive::kPrimInt).ToString().c_str());
   SideEffects s = SideEffects::None();
   s = s.Union(SideEffects::FieldWriteOfType(Primitive::kPrimChar, /* is_volatile */ false));
   s = s.Union(SideEffects::FieldWriteOfType(Primitive::kPrimLong, /* is_volatile */ false));
@@ -249,7 +231,9 @@ TEST(SideEffectsTest, BitStrings) {
   s = s.Union(SideEffects::FieldReadOfType(Primitive::kPrimInt, /* is_volatile */ false));
   s = s.Union(SideEffects::ArrayReadOfType(Primitive::kPrimFloat));
   s = s.Union(SideEffects::ArrayReadOfType(Primitive::kPrimDouble));
-  EXPECT_STREQ("||DF|I||S|JC|", s.ToString().c_str());
+  EXPECT_STREQ(
+      "||DFJI|FI||S|DJC|",   // note: DJ/FI alias.
+      s.ToString().c_str());
 }
 
 }  // namespace art

diff --git a/test/525-checker-arrays-and-fields/expected.txt b/test/525-checker-arrays-and-fields/expected.txt
index b0aad4d..e69de29 100644 (file)
--- a/test/525-checker-arrays-and-fields/expected.txt
+++ b/test/525-checker-arrays-and-fields/expected.txt
@@ -1 +0,0 @@
-passed

diff --git a/test/525-checker-arrays-and-fields/src/Main.java b/test/525-checker-arrays-and-fields/src/Main.java
index b3fbf44..a635a51 100644 (file)
--- a/test/525-checker-arrays-and-fields/src/Main.java
+++ b/test/525-checker-arrays-and-fields/src/Main.java
@@ -759,87 +759,12 @@ public class Main {
   }
 
   //
-  // Misc. tests on false cross-over loops with data types (I/F and J/D) that used
-  // to be aliased in an older version of the compiler. This alias has been removed,
-  // however, which enables hoisting the invariant array reference.
-  //
-
-  /// CHECK-START: void Main.FalseCrossOverLoop1() licm (before)
-  /// CHECK-DAG: ArraySet loop:none
-  /// CHECK-DAG: ArrayGet loop:{{B\d+}}
-  /// CHECK-DAG: ArraySet loop:{{B\d+}}
-
-  /// CHECK-START: void Main.FalseCrossOverLoop1() licm (after)
-  /// CHECK-DAG: ArraySet loop:none
-  /// CHECK-DAG: ArrayGet loop:none
-  /// CHECK-DAG: ArraySet loop:{{B\d+}}
-
-  private static void FalseCrossOverLoop1() {
-    sArrF[20] = -1;
-    for (int i = 0; i < sArrI.length; i++) {
-      sArrI[i] = (int) sArrF[20] - 2;
-    }
-  }
-
-  /// CHECK-START: void Main.FalseCrossOverLoop2() licm (before)
-  /// CHECK-DAG: ArraySet loop:none
-  /// CHECK-DAG: ArrayGet loop:{{B\d+}}
-  /// CHECK-DAG: ArraySet loop:{{B\d+}}
-
-  /// CHECK-START: void Main.FalseCrossOverLoop2() licm (after)
-  /// CHECK-DAG: ArraySet loop:none
-  /// CHECK-DAG: ArrayGet loop:none
-  /// CHECK-DAG: ArraySet loop:{{B\d+}}
-
-  private static void FalseCrossOverLoop2() {
-    sArrI[20] = -2;
-    for (int i = 0; i < sArrF.length; i++) {
-      sArrF[i] = sArrI[20] - 2;
-    }
-  }
-
-  /// CHECK-START: void Main.FalseCrossOverLoop3() licm (before)
-  /// CHECK-DAG: ArraySet loop:none
-  /// CHECK-DAG: ArrayGet loop:{{B\d+}}
-  /// CHECK-DAG: ArraySet loop:{{B\d+}}
-
-  /// CHECK-START: void Main.FalseCrossOverLoop3() licm (after)
-  /// CHECK-DAG: ArraySet loop:none
-  /// CHECK-DAG: ArrayGet loop:none
-  /// CHECK-DAG: ArraySet loop:{{B\d+}}
-
-  private static void FalseCrossOverLoop3() {
-    sArrD[20] = -3;
-    for (int i = 0; i < sArrJ.length; i++) {
-      sArrJ[i] = (long) sArrD[20] - 2;
-    }
-  }
-
-  /// CHECK-START: void Main.FalseCrossOverLoop4() licm (before)
-  /// CHECK-DAG: ArraySet loop:none
-  /// CHECK-DAG: ArrayGet loop:{{B\d+}}
-  /// CHECK-DAG: ArraySet loop:{{B\d+}}
-
-  /// CHECK-START: void Main.FalseCrossOverLoop4() licm (after)
-  /// CHECK-DAG: ArraySet loop:none
-  /// CHECK-DAG: ArrayGet loop:none
-  /// CHECK-DAG: ArraySet loop:{{B\d+}}
-
-  private static void FalseCrossOverLoop4() {
-    sArrJ[20] = -4;
-    for (int i = 0; i < sArrD.length; i++) {
-      sArrD[i] = sArrJ[20] - 2;
-    }
-  }
-
-  //
   // Driver and testers.
   //
 
   public static void main(String[] args) {
     DoStaticTests();
     new Main().DoInstanceTests();
-    System.out.println("passed");
   }
 
   private static void DoStaticTests() {
@@ -978,23 +903,6 @@ public class Main {
     for (int i = 0; i < sArrL.length; i++) {
       expectEquals(i <= 20 ? anObject : anotherObject, sArrL[i]);
     }
-    // Misc. tests.
-    FalseCrossOverLoop1();
-    for (int i = 0; i < sArrI.length; i++) {
-      expectEquals(-3, sArrI[i]);
-    }
-    FalseCrossOverLoop2();
-    for (int i = 0; i < sArrF.length; i++) {
-      expectEquals(-4, sArrF[i]);
-    }
-    FalseCrossOverLoop3();
-    for (int i = 0; i < sArrJ.length; i++) {
-      expectEquals(-5, sArrJ[i]);
-    }
-    FalseCrossOverLoop4();
-    for (int i = 0; i < sArrD.length; i++) {
-      expectEquals(-6, sArrD[i]);
-    }
   }
 
   private void DoInstanceTests() {