From 0bcb607868af01cdd980900481c5cd012caeb457 Mon Sep 17 00:00:00 2001 From: Teagan Strickland Date: Thu, 6 Jun 2019 13:22:09 +0000 Subject: [PATCH] [vm/compiler] Reland "Fix receiver redefinition type for poly inlining." In cases where the same target function is used for multiple CID ranges, we only perform the inlining once. However, if the CID range used for the initial inlining is a single CID, we will set the type of the redefinition to that CID. Detect this case and clear the concrete type associated with the redefinition if it was given one. Change-Id: I9f9bdd7c21e0dc1ac537f8facece0010630bd9aa Cq-Include-Trybots: luci.dart.try:vm-kernel-win-debug-x64-try,vm-kernel-win-release-ia32-try,vm-kernel-win-product-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105221 Commit-Queue: Teagan Strickland Reviewed-by: Martin Kustermann --- runtime/vm/compiler/backend/inliner.cc | 5 + runtime/vm/compiler/backend/inliner_test.cc | 137 ++++++++++++++++++++ runtime/vm/compiler/compiler_sources.gni | 1 + 3 files changed, 143 insertions(+) create mode 100644 runtime/vm/compiler/backend/inliner_test.cc diff --git a/runtime/vm/compiler/backend/inliner.cc b/runtime/vm/compiler/backend/inliner.cc index 7be5d0ec0f91..9c17bb46404a 100644 --- a/runtime/vm/compiler/backend/inliner.cc +++ b/runtime/vm/compiler/backend/inliner.cc @@ -1731,6 +1731,11 @@ bool PolymorphicInliner::CheckInlinedDuplicate(const Function& target) { BlockEntryInstr* block = old_target->dominated_blocks()[j]; new_join->AddDominatedBlock(block); } + // Since we are reusing the same inlined body across multiple cids, + // reset the type information on the redefinition of the receiver + // in case it was originally given a concrete type. + ASSERT(new_join->next()->IsRedefinition()); + new_join->next()->AsRedefinition()->UpdateType(CompileType::Dynamic()); // Create a new target with the join as unconditional successor. TargetEntryInstr* new_target = new TargetEntryInstr( AllocateBlockId(), old_target->try_index(), DeoptId::kNone); diff --git a/runtime/vm/compiler/backend/inliner_test.cc b/runtime/vm/compiler/backend/inliner_test.cc new file mode 100644 index 000000000000..75609a4632a7 --- /dev/null +++ b/runtime/vm/compiler/backend/inliner_test.cc @@ -0,0 +1,137 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#include "vm/compiler/backend/inliner.h" + +#include "vm/compiler/backend/il.h" +#include "vm/compiler/backend/il_printer.h" +#include "vm/compiler/backend/il_test_helper.h" +#include "vm/compiler/compiler_pass.h" +#include "vm/object.h" +#include "vm/unit_test.h" + +namespace dart { + +// Test that the redefinition for an inlined polymorphic function used with +// multiple receiver cids does not have a concrete type. +ISOLATE_UNIT_TEST_CASE(Inliner_PolyInliningRedefinition) { + const char* kScript = R"( + abstract class A { + String toInline() { return "A"; } + } + + class B extends A {} + class C extends A { + @override + String toInline() { return "C";} + } + class D extends A {} + + testInlining(A arg) { + arg.toInline(); + } + + main() { + for (var i = 0; i < 10; i++) { + testInlining(B()); + testInlining(C()); + testInlining(D()); + } + } + )"; + + const auto& root_library = Library::Handle(LoadTestScript(kScript)); + const auto& function = + Function::Handle(GetFunction(root_library, "testInlining")); + + Invoke(root_library, "main"); + + TestPipeline pipeline(function, CompilerPass::kJIT); + FlowGraph* flow_graph = pipeline.RunPasses({ + CompilerPass::kComputeSSA, + CompilerPass::kApplyICData, + CompilerPass::kTryOptimizePatterns, + CompilerPass::kSetOuterInliningId, + CompilerPass::kTypePropagation, + CompilerPass::kApplyClassIds, + CompilerPass::kInlining, + }); + + auto entry = flow_graph->graph_entry()->normal_entry(); + EXPECT(entry != nullptr); + + EXPECT(entry->initial_definitions()->length() == 1); + EXPECT(entry->initial_definitions()->At(0)->IsParameter()); + ParameterInstr* param = entry->initial_definitions()->At(0)->AsParameter(); + + // First we find the start of the prelude for the inlined instruction, + // and also keep a reference to the LoadClassId instruction for later. + LoadClassIdInstr* lcid = nullptr; + BranchInstr* prelude = nullptr; + + ILMatcher cursor(flow_graph, entry); + RELEASE_ASSERT(cursor.TryMatch( + { + {kMatchLoadClassId, &lcid}, + {kMatchBranch, &prelude}, + }, + /*insert_before=*/kMoveGlob)); + + const Class& cls = Class::Handle( + root_library.LookupLocalClass(String::Handle(Symbols::New(thread, "B")))); + + Definition* cid_B = flow_graph->GetConstant(Smi::Handle(Smi::New(cls.id()))); + Instruction* current = prelude; + + // We walk false branches until we either reach a branch instruction that uses + // B's cid for comparison to the value returned from the LCID instruction + // above, or a default case if there was no branch instruction for B's cid. + while (true) { + EXPECT(current->IsBranch()); + const ComparisonInstr* check = current->AsBranch()->comparison(); + EXPECT(check->left()->definition() == lcid); + if (check->right()->definition() == cid_B) break; + current = current->SuccessorAt(1); + // By following false paths, we should be walking a series of blocks that + // looks like: + // B#[target]:# + // Branch if + // If we end up not finding a branch, then we're in a default case + // that contains a class check. + current = current->next(); + if (!current->IsBranch()) { + break; + } + } + // If we found a branch that checks against the class ID, we follow the true + // branch to a block that contains only a goto to the desired join block. + if (current->IsBranch()) { + current = current->SuccessorAt(0); + } else { + // We're in the default case, which will check the class ID to make sure + // it's the one expected for the fallthrough. That check will be followed + // by a goto to the desired join block. + EXPECT(current->IsRedefinition()); + const auto redef = current->AsRedefinition(); + EXPECT(redef->value()->definition() == lcid); + current = current->next(); + EXPECT(current->IsCheckClassId()); + EXPECT(current->AsCheckClassId()->value()->definition() == redef); + } + current = current->next(); + EXPECT(current->IsGoto()); + current = current->AsGoto()->successor(); + // Now we should be at a block that starts like: + // BY[join]:# pred(...) + // vW <- Redefinition(vV) + // + // where vV is a reference to the function parameter (the receiver of + // the inlined function). + current = current->next(); + EXPECT(current->IsRedefinition()); + EXPECT(current->AsRedefinition()->value()->definition() == param); + EXPECT(current->AsRedefinition()->Type()->ToCid() == kDynamicCid); +} + +} // namespace dart diff --git a/runtime/vm/compiler/compiler_sources.gni b/runtime/vm/compiler/compiler_sources.gni index 632e5b40c096..bac526c10f12 100644 --- a/runtime/vm/compiler/compiler_sources.gni +++ b/runtime/vm/compiler/compiler_sources.gni @@ -168,6 +168,7 @@ compiler_sources_tests = [ "backend/il_test.cc", "backend/il_test_helper.h", "backend/il_test_helper.cc", + "backend/inliner_test.cc", "backend/locations_helpers_test.cc", "backend/loops_test.cc", "backend/range_analysis_test.cc",