From 1f7624c3bc41251ff72b1409441f541d992967c7 Mon Sep 17 00:00:00 2001 From: Aart Bik Date: Wed, 13 Apr 2016 21:17:17 +0000 Subject: 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 --- compiler/optimizing/licm_test.cc | 12 +++++++---- compiler/optimizing/nodes.h | 33 +++++++++++++++++++++++-------- compiler/optimizing/side_effects_test.cc | 34 +++++++++----------------------- 3 files changed, 42 insertions(+), 37 deletions(-) (limited to 'compiler/optimizing') diff --git a/compiler/optimizing/licm_test.cc b/compiler/optimizing/licm_test.cc index 2a62643465..d446539700 100644 --- 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 6f3e536d05..dc5a8fa9cb 100644 --- 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(expected_component_type); SetPackedFlag(value->GetType() == Primitive::kPrimNot); SetPackedFlag(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 b01bc1ca0d..9bbc354290 100644 --- 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)); } @@ -215,33 +215,15 @@ TEST(SideEffectsTest, BitStrings) { EXPECT_STREQ( "||||||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 -- cgit v1.2.3-59-g8ed1b