Fix broken tests.

Rationale:
  (1) volatile field write/read need to apply to all
      to comply with Java memory model
  (2) clinit only needs only the write
  (3) added conservative assumptions to memory barrier
      (nothing broke, but this seems better)

Change-Id: I37787ec8f3f2c8d6166a94c57193fa4544ad3372

diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index badb2bf..85aa004 100644 (file)
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -1213,16 +1213,28 @@ class SideEffects : public ValueObject {
     return SideEffects(kAllWrites | kAllReads);
   }
 
-  static SideEffects FieldWriteOfType(Primitive::Type type) {
-    return SideEffects(TypeFlagWithAlias(type, kFieldWriteOffset));
+  static SideEffects AllWrites() {
+    return SideEffects(kAllWrites);
+  }
+
+  static SideEffects AllReads() {
+    return SideEffects(kAllReads);
+  }
+
+  static SideEffects FieldWriteOfType(Primitive::Type type, bool is_volatile) {
+    return is_volatile
+        ? All()
+        : SideEffects(TypeFlagWithAlias(type, kFieldWriteOffset));
   }
 
   static SideEffects ArrayWriteOfType(Primitive::Type type) {
     return SideEffects(TypeFlagWithAlias(type, kArrayWriteOffset));
   }
 
-  static SideEffects FieldReadOfType(Primitive::Type type) {
-    return SideEffects(TypeFlagWithAlias(type, kFieldReadOffset));
+  static SideEffects FieldReadOfType(Primitive::Type type, bool is_volatile) {
+    return is_volatile
+        ? All()
+        : SideEffects(TypeFlagWithAlias(type, kFieldReadOffset));
   }
 
   static SideEffects ArrayReadOfType(Primitive::Type type) {
@@ -3593,7 +3605,9 @@ class HInstanceFieldGet : public HExpression<1> {
                     bool is_volatile,
                     uint32_t field_idx,
                     const DexFile& dex_file)
-      : HExpression(field_type, SideEffects::SideEffects::FieldReadOfType(field_type)),
+      : HExpression(
+            field_type,
+            SideEffects::SideEffects::FieldReadOfType(field_type, is_volatile)),
         field_info_(field_offset, field_type, is_volatile, field_idx, dex_file) {
     SetRawInputAt(0, value);
   }
@@ -3635,7 +3649,8 @@ class HInstanceFieldSet : public HTemplateInstruction<2> {
                     bool is_volatile,
                     uint32_t field_idx,
                     const DexFile& dex_file)
-      : HTemplateInstruction(SideEffects::FieldWriteOfType(field_type)),
+      : HTemplateInstruction(
+          SideEffects::FieldWriteOfType(field_type, is_volatile)),
         field_info_(field_offset, field_type, is_volatile, field_idx, dex_file),
         value_can_be_null_(true) {
     SetRawInputAt(0, object);
@@ -4005,7 +4020,7 @@ class HClinitCheck : public HExpression<1> {
   explicit HClinitCheck(HLoadClass* constant, uint32_t dex_pc)
       : HExpression(
           Primitive::kPrimNot,
-          SideEffects::All()),  // assume write/read on all fields/arrays
+          SideEffects::AllWrites()),  // assume write on all fields/arrays
         dex_pc_(dex_pc) {
     SetRawInputAt(0, constant);
   }
@@ -4041,7 +4056,9 @@ class HStaticFieldGet : public HExpression<1> {
                   bool is_volatile,
                   uint32_t field_idx,
                   const DexFile& dex_file)
-      : HExpression(field_type, SideEffects::SideEffects::FieldReadOfType(field_type)),
+      : HExpression(
+            field_type,
+            SideEffects::SideEffects::FieldReadOfType(field_type, is_volatile)),
         field_info_(field_offset, field_type, is_volatile, field_idx, dex_file) {
     SetRawInputAt(0, cls);
   }
@@ -4080,7 +4097,8 @@ class HStaticFieldSet : public HTemplateInstruction<2> {
                   bool is_volatile,
                   uint32_t field_idx,
                   const DexFile& dex_file)
-      : HTemplateInstruction(SideEffects::FieldWriteOfType(field_type)),
+      : HTemplateInstruction(
+          SideEffects::FieldWriteOfType(field_type, is_volatile)),
         field_info_(field_offset, field_type, is_volatile, field_idx, dex_file),
         value_can_be_null_(true) {
     SetRawInputAt(0, cls);
@@ -4255,7 +4273,8 @@ class HCheckCast : public HTemplateInstruction<2> {
 class HMemoryBarrier : public HTemplateInstruction<0> {
  public:
   explicit HMemoryBarrier(MemBarrierKind barrier_kind)
-      : HTemplateInstruction(SideEffects::None()),
+      : HTemplateInstruction(
+          SideEffects::All()),  // assume write/read on all fields/arrays
         barrier_kind_(barrier_kind) {}
 
   MemBarrierKind GetBarrierKind() { return barrier_kind_; }

diff --git a/compiler/optimizing/side_effects_test.cc b/compiler/optimizing/side_effects_test.cc
index 813f7ce..8db5a8a 100644 (file)
--- a/compiler/optimizing/side_effects_test.cc
+++ b/compiler/optimizing/side_effects_test.cc
@@ -95,50 +95,71 @@ TEST(SideEffectsTest, DependencesAndNoDependences) {
       type = Primitive::Type(type + 1)) {
     // Same primitive type and access type: proper write/read dep.
     testWriteAndReadDependence(
-        SideEffects::FieldWriteOfType(type),
-        SideEffects::FieldReadOfType(type));
+        SideEffects::FieldWriteOfType(type, false),
+        SideEffects::FieldReadOfType(type, false));
     testWriteAndReadDependence(
         SideEffects::ArrayWriteOfType(type),
         SideEffects::ArrayReadOfType(type));
     // Same primitive type but different access type: no write/read dep.
     testNoWriteAndReadDependence(
-        SideEffects::FieldWriteOfType(type),
+        SideEffects::FieldWriteOfType(type, false),
         SideEffects::ArrayReadOfType(type));
     testNoWriteAndReadDependence(
         SideEffects::ArrayWriteOfType(type),
-        SideEffects::FieldReadOfType(type));
+        SideEffects::FieldReadOfType(type, false));
   }
 }
 
 TEST(SideEffectsTest, NoDependences) {
   // Different primitive type, same access type: no write/read dep.
   testNoWriteAndReadDependence(
-      SideEffects::FieldWriteOfType(Primitive::kPrimInt),
-      SideEffects::FieldReadOfType(Primitive::kPrimDouble));
+      SideEffects::FieldWriteOfType(Primitive::kPrimInt, false),
+      SideEffects::FieldReadOfType(Primitive::kPrimDouble, false));
   testNoWriteAndReadDependence(
       SideEffects::ArrayWriteOfType(Primitive::kPrimInt),
       SideEffects::ArrayReadOfType(Primitive::kPrimDouble));
   // Everything different: no write/read dep.
   testNoWriteAndReadDependence(
-      SideEffects::FieldWriteOfType(Primitive::kPrimInt),
+      SideEffects::FieldWriteOfType(Primitive::kPrimInt, false),
       SideEffects::ArrayReadOfType(Primitive::kPrimDouble));
   testNoWriteAndReadDependence(
       SideEffects::ArrayWriteOfType(Primitive::kPrimInt),
-      SideEffects::FieldReadOfType(Primitive::kPrimDouble));
+      SideEffects::FieldReadOfType(Primitive::kPrimDouble, false));
+}
+
+TEST(SideEffectsTest, VolatileDependences) {
+  SideEffects volatile_write =
+      SideEffects::FieldWriteOfType(Primitive::kPrimInt, true);
+  SideEffects any_write =
+      SideEffects::FieldWriteOfType(Primitive::kPrimInt, false);
+  SideEffects volatile_read =
+      SideEffects::FieldReadOfType(Primitive::kPrimByte, true);
+  SideEffects any_read =
+      SideEffects::FieldReadOfType(Primitive::kPrimByte, false);
+
+  EXPECT_FALSE(volatile_write.MayDependOn(any_read));
+  EXPECT_TRUE(any_read.MayDependOn(volatile_write));
+  EXPECT_TRUE(volatile_write.MayDependOn(any_write));
+  EXPECT_FALSE(any_write.MayDependOn(volatile_write));
+
+  EXPECT_FALSE(volatile_read.MayDependOn(any_read));
+  EXPECT_TRUE(any_read.MayDependOn(volatile_read));
+  EXPECT_TRUE(volatile_read.MayDependOn(any_write));
+  EXPECT_FALSE(any_write.MayDependOn(volatile_read));
 }
 
 TEST(SideEffectsTest, SameWidthTypes) {
   // Type I/F.
   testWriteAndReadDependence(
-      SideEffects::FieldWriteOfType(Primitive::kPrimInt),
-      SideEffects::FieldReadOfType(Primitive::kPrimFloat));
+      SideEffects::FieldWriteOfType(Primitive::kPrimInt, false),
+      SideEffects::FieldReadOfType(Primitive::kPrimFloat, false));
   testWriteAndReadDependence(
       SideEffects::ArrayWriteOfType(Primitive::kPrimInt),
       SideEffects::ArrayReadOfType(Primitive::kPrimFloat));
   // Type L/D.
   testWriteAndReadDependence(
-      SideEffects::FieldWriteOfType(Primitive::kPrimLong),
-      SideEffects::FieldReadOfType(Primitive::kPrimDouble));
+      SideEffects::FieldWriteOfType(Primitive::kPrimLong, false),
+      SideEffects::FieldReadOfType(Primitive::kPrimDouble, false));
   testWriteAndReadDependence(
       SideEffects::ArrayWriteOfType(Primitive::kPrimLong),
       SideEffects::ArrayReadOfType(Primitive::kPrimDouble));
@@ -150,9 +171,9 @@ TEST(SideEffectsTest, AllWritesAndReads) {
   for (Primitive::Type type = Primitive::kPrimNot;
         type < Primitive::kPrimVoid;
         type = Primitive::Type(type + 1)) {
-    s = s.Union(SideEffects::FieldWriteOfType(type));
+    s = s.Union(SideEffects::FieldWriteOfType(type, false));
     s = s.Union(SideEffects::ArrayWriteOfType(type));
-    s = s.Union(SideEffects::FieldReadOfType(type));
+    s = s.Union(SideEffects::FieldReadOfType(type, false));
     s = s.Union(SideEffects::ArrayReadOfType(type));
   }
   EXPECT_TRUE(s.DoesAll());
@@ -166,22 +187,28 @@ TEST(SideEffectsTest, BitStrings) {
       "|DFJISCBZL|DFJISCBZL|DFJISCBZL|DFJISCBZL|",
       SideEffects::All().ToString().c_str());
   EXPECT_STREQ(
+      "|||DFJISCBZL|DFJISCBZL|",
+      SideEffects::AllWrites().ToString().c_str());
+  EXPECT_STREQ(
+      "|DFJISCBZL|DFJISCBZL|||",
+      SideEffects::AllReads().ToString().c_str());
+  EXPECT_STREQ(
       "||||L|",
-      SideEffects::FieldWriteOfType(Primitive::kPrimNot).ToString().c_str());
+      SideEffects::FieldWriteOfType(Primitive::kPrimNot, false).ToString().c_str());
   EXPECT_STREQ(
       "|||Z||",
       SideEffects::ArrayWriteOfType(Primitive::kPrimBoolean).ToString().c_str());
   EXPECT_STREQ(
       "||B|||",
-      SideEffects::FieldReadOfType(Primitive::kPrimByte).ToString().c_str());
+      SideEffects::FieldReadOfType(Primitive::kPrimByte, false).ToString().c_str());
   EXPECT_STREQ(
       "|DJ||||",  // note: DJ alias
       SideEffects::ArrayReadOfType(Primitive::kPrimDouble).ToString().c_str());
   SideEffects s = SideEffects::None();
-  s = s.Union(SideEffects::FieldWriteOfType(Primitive::kPrimChar));
-  s = s.Union(SideEffects::FieldWriteOfType(Primitive::kPrimLong));
+  s = s.Union(SideEffects::FieldWriteOfType(Primitive::kPrimChar, false));
+  s = s.Union(SideEffects::FieldWriteOfType(Primitive::kPrimLong, false));
   s = s.Union(SideEffects::ArrayWriteOfType(Primitive::kPrimShort));
-  s = s.Union(SideEffects::FieldReadOfType(Primitive::kPrimInt));
+  s = s.Union(SideEffects::FieldReadOfType(Primitive::kPrimInt, false));
   s = s.Union(SideEffects::ArrayReadOfType(Primitive::kPrimFloat));
   s = s.Union(SideEffects::ArrayReadOfType(Primitive::kPrimDouble));
   EXPECT_STREQ(