From 34c3ba93e74d14ab832297ff590cb76c3f0f519d Mon Sep 17 00:00:00 2001 From: Aart Bik Date: Mon, 20 Jul 2015 14:08:59 -0700 Subject: [PATCH] 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 --- compiler/optimizing/nodes.h | 39 ++++++++++++++----- compiler/optimizing/side_effects_test.cc | 65 ++++++++++++++++++++++---------- 2 files changed, 75 insertions(+), 29 deletions(-) diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index badb2bfe1..85aa0040c 100644 --- 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 813f7cec1..8db5a8a35 100644 --- 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( -- 2.11.0