From 6daf74f845075b85f02ca57cf4367cefdeca9db3 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Tue, 4 Jan 2022 11:57:45 +0000 Subject: [PATCH] [vm/compiler] Unbox more integer phis on 32bit targets Replace previous more conservative heuristic with a new one which unboxes all the phis which have at least one unboxed input and at least one unboxed use. Additionally the new heuristic looks through phis transitively when looking for inputs and uses. TEST=ci Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-simarm_x64-try,vm-kernel-precomp-linux-release-simarm-try,vm-kernel-precomp-linux-release-simarm64-try Change-Id: I5cb3280187fb29be77400e9fc4ead1cb6a6c3748 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/224202 Reviewed-by: Alexander Markov Commit-Queue: Slava Egorov --- runtime/vm/compiler/backend/flow_graph.cc | 227 ++++++++++++---------- runtime/vm/compiler/backend/il.h | 125 ++++++++---- runtime/vm/compiler/backend/loops_test.cc | 16 -- 3 files changed, 214 insertions(+), 154 deletions(-) diff --git a/runtime/vm/compiler/backend/flow_graph.cc b/runtime/vm/compiler/backend/flow_graph.cc index 1dbf2136258..22ff2d3530b 100644 --- a/runtime/vm/compiler/backend/flow_graph.cc +++ b/runtime/vm/compiler/backend/flow_graph.cc @@ -1954,136 +1954,153 @@ void FlowGraph::InsertConversionsFor(Definition* def) { } } -static void UnboxPhi(PhiInstr* phi, bool is_aot) { - Representation unboxed = phi->representation(); +namespace { +class PhiUnboxingHeuristic : public ValueObject { + public: + explicit PhiUnboxingHeuristic(FlowGraph* flow_graph) + : worklist_(flow_graph, 10) {} - switch (phi->Type()->ToCid()) { - case kDoubleCid: - if (CanUnboxDouble()) { - unboxed = kUnboxedDouble; - } - break; - case kFloat32x4Cid: - if (ShouldInlineSimd()) { - unboxed = kUnboxedFloat32x4; - } - break; - case kInt32x4Cid: - if (ShouldInlineSimd()) { - unboxed = kUnboxedInt32x4; - } - break; - case kFloat64x2Cid: - if (ShouldInlineSimd()) { - unboxed = kUnboxedFloat64x2; - } - break; - } + void Process(PhiInstr* phi) { + Representation unboxed = phi->representation(); - // If all the inputs are unboxed, leave the Phi unboxed. - if ((unboxed == kTagged) && phi->Type()->IsInt()) { - bool should_unbox = true; - Representation new_representation = kTagged; - for (intptr_t i = 0; i < phi->InputCount(); i++) { - Definition* input = phi->InputAt(i)->definition(); - if (input->representation() != kUnboxedInt64 && - input->representation() != kUnboxedInt32 && - input->representation() != kUnboxedUint32 && !(input == phi)) { - should_unbox = false; + switch (phi->Type()->ToCid()) { + case kDoubleCid: + if (CanUnboxDouble()) { + unboxed = kUnboxedDouble; + } break; - } - - if (new_representation == kTagged) { - new_representation = input->representation(); - } else if (new_representation != input->representation()) { - new_representation = kNoRepresentation; - } - } - if (should_unbox) { - unboxed = new_representation != kNoRepresentation - ? new_representation - : RangeUtils::Fits(phi->range(), - RangeBoundary::kRangeBoundaryInt32) - ? kUnboxedInt32 - : kUnboxedInt64; - } - } - - if ((unboxed == kTagged) && phi->Type()->IsInt() && - !phi->Type()->can_be_sentinel()) { - // Conservatively unbox phis that: - // - are proven to be of type Int; - // - fit into 64bits range; - // - have either constants or Box() operations as inputs; - // - have at least one Box() operation as an input; - // - are used in at least 1 Unbox() operation. - bool should_unbox = false; - for (intptr_t i = 0; i < phi->InputCount(); i++) { - Definition* input = phi->InputAt(i)->definition(); - if (input->IsBox()) { - should_unbox = true; - } else if (!input->IsConstant()) { - should_unbox = false; + case kFloat32x4Cid: + if (ShouldInlineSimd()) { + unboxed = kUnboxedFloat32x4; + } + break; + case kInt32x4Cid: + if (ShouldInlineSimd()) { + unboxed = kUnboxedInt32x4; + } + break; + case kFloat64x2Cid: + if (ShouldInlineSimd()) { + unboxed = kUnboxedFloat64x2; + } break; - } } - if (should_unbox) { - // We checked inputs. Check if phi is used in at least one unbox - // operation. - bool has_unboxed_use = false; - for (Value* use = phi->input_use_list(); use != NULL; - use = use->next_use()) { - Instruction* instr = use->instruction(); - if (instr->IsUnbox()) { - has_unboxed_use = true; - break; - } else if (IsUnboxedInteger( - instr->RequiredInputRepresentation(use->use_index()))) { - has_unboxed_use = true; + // If all the inputs are unboxed, leave the Phi unboxed. + if ((unboxed == kTagged) && phi->Type()->IsInt()) { + bool should_unbox = true; + Representation new_representation = kTagged; + for (auto input : phi->inputs()) { + if (input == phi) continue; + + if (!IsUnboxedInteger(input->representation())) { + should_unbox = false; break; } + + if (new_representation == kTagged) { + new_representation = input->representation(); + } else if (new_representation != input->representation()) { + new_representation = kNoRepresentation; + } } - if (!has_unboxed_use) { - should_unbox = false; + if (should_unbox) { + unboxed = + new_representation != kNoRepresentation ? new_representation + : RangeUtils::Fits(phi->range(), RangeBoundary::kRangeBoundaryInt32) + ? kUnboxedInt32 + : kUnboxedInt64; } } - if (should_unbox) { - unboxed = - RangeUtils::Fits(phi->range(), RangeBoundary::kRangeBoundaryInt32) - ? kUnboxedInt32 - : kUnboxedInt64; - } - } - + // Decide if it is worth to unbox an integer phi. + if ((unboxed == kTagged) && phi->Type()->IsInt() && + !phi->Type()->can_be_sentinel()) { #if defined(TARGET_ARCH_IS_64_BIT) - // In AOT mode on 64-bit platforms always unbox integer typed phis (similar - // to how we treat doubles and other boxed numeric types). - // In JIT mode only unbox phis which are not fully known to be Smi. - if ((unboxed == kTagged) && phi->Type()->IsInt() && - !phi->Type()->can_be_sentinel() && - (is_aot || phi->Type()->ToCid() != kSmiCid)) { - unboxed = kUnboxedInt64; - } -#endif + // In AOT mode on 64-bit platforms always unbox integer typed phis + // (similar to how we treat doubles and other boxed numeric types). + // In JIT mode only unbox phis which are not fully known to be Smi. + if (is_aot_ || phi->Type()->ToCid() != kSmiCid) { + unboxed = kUnboxedInt64; + } +#else + // If we are on a 32-bit platform check if there are unboxed values + // flowing into the phi and the phi value itself is flowing into an + // unboxed operation prefer to keep it unboxed. + // We use this heuristic instead of eagerly unboxing all the phis + // because we are concerned about the code size and register pressure. + const bool has_unboxed_incomming_value = HasUnboxedIncommingValue(phi); + const bool flows_into_unboxed_use = FlowsIntoUnboxedUse(phi); - phi->set_representation(unboxed); -} + if (has_unboxed_incomming_value && flows_into_unboxed_use) { + unboxed = + RangeUtils::Fits(phi->range(), RangeBoundary::kRangeBoundaryInt32) + ? kUnboxedInt32 + : kUnboxedInt64; + } +#endif + } + + phi->set_representation(unboxed); + } + + private: + // Returns |true| iff there is an unboxed definition among all potential + // definitions that can flow into the |phi|. + // This function looks through phis. + bool HasUnboxedIncommingValue(PhiInstr* phi) { + worklist_.Clear(); + worklist_.Add(phi); + for (intptr_t i = 0; i < worklist_.definitions().length(); i++) { + const auto defn = worklist_.definitions()[i]; + for (auto input : defn->inputs()) { + if (IsUnboxedInteger(input->representation()) || input->IsBox()) { + return true; + } else if (input->IsPhi()) { + worklist_.Add(input); + } + } + } + return false; + } + + // Returns |true| iff |phi| potentially flows into an unboxed use. + // This function looks through phis. + bool FlowsIntoUnboxedUse(PhiInstr* phi) { + worklist_.Clear(); + worklist_.Add(phi); + for (intptr_t i = 0; i < worklist_.definitions().length(); i++) { + const auto defn = worklist_.definitions()[i]; + for (auto use : defn->input_uses()) { + if (IsUnboxedInteger(use->instruction()->RequiredInputRepresentation( + use->use_index())) || + use->instruction()->IsUnbox()) { + return true; + } else if (auto phi_use = use->instruction()->AsPhi()) { + worklist_.Add(phi_use); + } + } + } + return false; + } + + const bool is_aot_ = CompilerState::Current().is_aot(); + DefinitionWorklist worklist_; +}; +} // namespace void FlowGraph::SelectRepresentations() { - const auto is_aot = CompilerState::Current().is_aot(); - // First we decide for each phi if it is beneficial to unbox it. If so, we // change it's `phi->representation()` + PhiUnboxingHeuristic phi_unboxing_heuristic(this); for (BlockIterator block_it = reverse_postorder_iterator(); !block_it.Done(); block_it.Advance()) { JoinEntryInstr* join_entry = block_it.Current()->AsJoinEntry(); if (join_entry != NULL) { for (PhiIterator it(join_entry); !it.Done(); it.Advance()) { PhiInstr* phi = it.Current(); - UnboxPhi(phi, is_aot); + phi_unboxing_heuristic.Process(phi); } } } diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h index c261131864b..0f832b97871 100644 --- a/runtime/vm/compiler/backend/il.h +++ b/runtime/vm/compiler/backend/il.h @@ -62,7 +62,6 @@ class ParsedFunction; class Range; class RangeAnalysis; class RangeBoundary; -class SuccessorsIterable; class TypeUsageInfo; class UnboxIntegerInstr; @@ -769,6 +768,64 @@ class BinaryFeedback : public ZoneAllocated { typedef ZoneGrowableArray InputsArray; typedef ZoneGrowableArray PushArgumentsArray; +template +class InstructionIndexedPropertyIterable { + public: + struct Iterator { + const Instruction* instr; + intptr_t index; + + decltype(Trait::At(instr, index)) operator*() const { + return Trait::At(instr, index); + } + Iterator& operator++() { + index++; + return *this; + } + + bool operator==(const Iterator& other) { + return instr == other.instr && index == other.index; + } + + bool operator!=(const Iterator& other) { return !(*this == other); } + }; + + explicit InstructionIndexedPropertyIterable(const Instruction* instr) + : instr_(instr) {} + + Iterator begin() const { return {instr_, 0}; } + Iterator end() const { return {instr_, Trait::Length(instr_)}; } + + private: + const Instruction* instr_; +}; + +class ValueListIterable { + public: + struct Iterator { + Value* value; + + Value* operator*() const { return value; } + + Iterator& operator++() { + value = value->next_use(); + return *this; + } + + bool operator==(const Iterator& other) { return value == other.value; } + + bool operator!=(const Iterator& other) { return !(*this == other); } + }; + + explicit ValueListIterable(Value* value) : value_(value) {} + + Iterator begin() const { return {value_}; } + Iterator end() const { return {nullptr}; } + + private: + Value* value_; +}; + class Instruction : public ZoneAllocated { public: #define DECLARE_TAG(type, attrs) k##type, @@ -828,6 +885,20 @@ class Instruction : public ZoneAllocated { RawSetInputAt(i, value); } + struct InputsTrait { + static Definition* At(const Instruction* instr, intptr_t index) { + return instr->InputAt(index)->definition(); + } + + static intptr_t Length(const Instruction* instr) { + return instr->InputCount(); + } + }; + + using InputsIterable = InstructionIndexedPropertyIterable; + + InputsIterable inputs() { return InputsIterable(this); } + // Remove all inputs (including in the environment) from their // definition's use lists. void UnuseAllInputs(); @@ -917,7 +988,22 @@ class Instruction : public ZoneAllocated { virtual intptr_t SuccessorCount() const; virtual BlockEntryInstr* SuccessorAt(intptr_t index) const; - inline SuccessorsIterable successors() const; + struct SuccessorsTrait { + static BlockEntryInstr* At(const Instruction* instr, intptr_t index) { + return instr->SuccessorAt(index); + } + + static intptr_t Length(const Instruction* instr) { + return instr->SuccessorCount(); + } + }; + + using SuccessorsIterable = + InstructionIndexedPropertyIterable; + + inline SuccessorsIterable successors() const { + return SuccessorsIterable(this); + } void Goto(JoinEntryInstr* entry); @@ -2297,6 +2383,10 @@ class Definition : public Instruction { Value* env_use_list() const { return env_use_list_; } void set_env_use_list(Value* head) { env_use_list_ = head; } + ValueListIterable input_uses() const { + return ValueListIterable(input_use_list_); + } + void AddInputUse(Value* value) { Value::AddToList(value, &input_use_list_); } void AddEnvUse(Value* value) { Value::AddToList(value, &env_use_list_); } @@ -9764,37 +9854,6 @@ inline bool Value::CanBe(const Object& value) { return (constant == nullptr) || constant->value().ptr() == value.ptr(); } -class SuccessorsIterable { - public: - struct Iterator { - const Instruction* instr; - intptr_t index; - - BlockEntryInstr* operator*() const { return instr->SuccessorAt(index); } - Iterator& operator++() { - index++; - return *this; - } - - bool operator==(const Iterator& other) { - return instr == other.instr && index == other.index; - } - - bool operator!=(const Iterator& other) { return !(*this == other); } - }; - - explicit SuccessorsIterable(const Instruction* instr) : instr_(instr) {} - - Iterator begin() const { return {instr_, 0}; } - Iterator end() const { return {instr_, instr_->SuccessorCount()}; } - - private: - const Instruction* instr_; -}; - -SuccessorsIterable Instruction::successors() const { - return SuccessorsIterable(this); -} } // namespace dart diff --git a/runtime/vm/compiler/backend/loops_test.cc b/runtime/vm/compiler/backend/loops_test.cc index 1722f3b82c9..dec8c0126ba 100644 --- a/runtime/vm/compiler/backend/loops_test.cc +++ b/runtime/vm/compiler/backend/loops_test.cc @@ -387,15 +387,7 @@ ISOLATE_UNIT_TEST_CASE(NonStrictConditionUpWrap) { const char* expected = " [0\n" " LIN(9223372036854775806 + 1 * i)\n" // phi -#if !defined(TARGET_ARCH_IS_64_BIT) - " LIN(9223372036854775806 + 1 * i)\n" // (un)boxing - " LIN(9223372036854775806 + 1 * i)\n" - " LIN(9223372036854775806 + 1 * i)\n" -#endif // !defined(TARGET_ARCH_IS_64_BIT) " LIN(9223372036854775807 + 1 * i)\n" // add -#if !defined(TARGET_ARCH_IS_64_BIT) - " LIN(9223372036854775807 + 1 * i)\n" // unbox -#endif // !defined(TARGET_ARCH_IS_64_BIT) " ]\n"; EXPECT_STREQ(expected, ComputeInduction(thread, script_chars)); } @@ -434,15 +426,7 @@ ISOLATE_UNIT_TEST_CASE(NonStrictConditionDownWrap) { const char* expected = " [0\n" " LIN(-9223372036854775807 + -1 * i)\n" // phi -#if !defined(TARGET_ARCH_IS_64_BIT) - " LIN(-9223372036854775807 + -1 * i)\n" // (un)boxing - " LIN(-9223372036854775807 + -1 * i)\n" - " LIN(-9223372036854775807 + -1 * i)\n" -#endif // !defined(TARGET_ARCH_IS_64_BIT) " LIN(-9223372036854775808 + -1 * i)\n" // sub -#if !defined(TARGET_ARCH_IS_64_BIT) - " LIN(-9223372036854775808 + -1 * i)\n" // unbox -#endif // !defined(TARGET_ARCH_IS_64_BIT) " ]\n"; EXPECT_STREQ(expected, ComputeInduction(thread, script_chars)); }