From 34fa3c1fb5da0751755061f35eebfd9c89dce09e Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Fri, 20 Sep 2024 14:47:49 +0000 Subject: [PATCH] [vm] Improve same-as-first constraint handling All other moves generated by the register allocator to satisfy constraints were using `PrefersRegister` for the source, but `SameAsFirstInput` was using `Any`. This lead to situations where hot code inside a loop would repeatedly reload a constant. To avoid these situations switch `SameAsFirstInput` to use `PrefersRegister` when the use occurs inside a loop. Additionally introduce an extension of `SameAsFirstInput`: `SameAsFirstOrSecondInput`. This new constraint allows register allocator to reorder first and second inputs if the second one is no longer alive after the instruction. This allows register allocator to avoid unnecessary move. `SameAsFirstOrSecondInput` can be used as an output constraint for commutative binary operations on X64 Additionally this CL adds a nascent infrastructure for writing unit tests against register allocator. See `linearscan_test.cc`. This CL improves code quality for tight loops with binary double operations written with constants on the left, for example: loop { doubleA = C * doubleB } Before this CL `C` would be reloaded into a register immediately before multiplication, but after this CL it will be kept in register (if register pressure allows). Issue https://github.com/dart-lang/sdk/issues/56705 TEST=LinearScan_TestSameAsFirstOrSecond* Change-Id: Id8e8242a8d1c1d1b8958076f10257e21e4a00aae Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/385001 Reviewed-by: Alexander Markov Commit-Queue: Slava Egorov --- .../compiler/backend/flow_graph_compiler.cc | 1 + runtime/vm/compiler/backend/il.cc | 27 +-- runtime/vm/compiler/backend/il.h | 1 + runtime/vm/compiler/backend/il_x64.cc | 12 +- runtime/vm/compiler/backend/linearscan.cc | 55 ++++- runtime/vm/compiler/backend/linearscan.h | 7 + .../vm/compiler/backend/linearscan_test.cc | 219 ++++++++++++++++++ runtime/vm/compiler/backend/locations.cc | 2 + runtime/vm/compiler/backend/locations.h | 9 + runtime/vm/compiler/compiler_sources.gni | 1 + runtime/vm/token.h | 17 ++ 11 files changed, 328 insertions(+), 23 deletions(-) create mode 100644 runtime/vm/compiler/backend/linearscan_test.cc diff --git a/runtime/vm/compiler/backend/flow_graph_compiler.cc b/runtime/vm/compiler/backend/flow_graph_compiler.cc index 0991fa362b3..2d64564e2ea 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler.cc @@ -1849,6 +1849,7 @@ void FlowGraphCompiler::AllocateRegistersLocally(Instruction* instr) { Location::RegisterLocation(AllocateFreeRegister(blocked_registers)); break; case Location::kSameAsFirstInput: + case Location::kSameAsFirstOrSecondInput: result_location = locs->in(0); break; case Location::kRequiresFpuRegister: diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc index 36a3b0ce298..99987218a85 100644 --- a/runtime/vm/compiler/backend/il.cc +++ b/runtime/vm/compiler/backend/il.cc @@ -2226,6 +2226,14 @@ Definition* BinaryDoubleOpInstr::Canonicalize(FlowGraph* flow_graph) { return square; } + if (left()->BindsToConstant() && !right()->BindsToConstant() && + Token::IsCommutativeOp(op_kind())) { + Value* l = left(); + Value* r = right(); + SetInputAt(0, r); + SetInputAt(1, l); + } + return this; } @@ -2233,23 +2241,6 @@ Definition* DoubleTestOpInstr::Canonicalize(FlowGraph* flow_graph) { return HasUses() ? this : nullptr; } -static bool IsCommutative(Token::Kind op) { - switch (op) { - case Token::kMUL: - FALL_THROUGH; - case Token::kADD: - FALL_THROUGH; - case Token::kBIT_AND: - FALL_THROUGH; - case Token::kBIT_OR: - FALL_THROUGH; - case Token::kBIT_XOR: - return true; - default: - return false; - } -} - UnaryIntegerOpInstr* UnaryIntegerOpInstr::Make(Representation representation, Token::Kind op_kind, Value* value, @@ -2430,7 +2421,7 @@ Definition* BinaryIntegerOpInstr::Canonicalize(FlowGraph* flow_graph) { } if (left()->BindsToConstant() && !right()->BindsToConstant() && - IsCommutative(op_kind())) { + Token::IsCommutativeOp(op_kind())) { Value* l = left(); Value* r = right(); SetInputAt(0, r); diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h index 466f4fb65b5..872fde35180 100644 --- a/runtime/vm/compiler/backend/il.h +++ b/runtime/vm/compiler/backend/il.h @@ -1733,6 +1733,7 @@ class BlockEntryInstr : public TemplateInstruction<0, NoThrow> { bool InsideTryBlock() const { return try_index_ != kInvalidTryIndex; } // Loop related methods. + bool IsInsideLoop() { return loop_info_ != nullptr; } LoopInfo* loop_info() const { return loop_info_; } void set_loop_info(LoopInfo* loop_info) { loop_info_ = loop_info; } bool IsLoopHeader() const; diff --git a/runtime/vm/compiler/backend/il_x64.cc b/runtime/vm/compiler/backend/il_x64.cc index d0ee56b6506..bf857863c7f 100644 --- a/runtime/vm/compiler/backend/il_x64.cc +++ b/runtime/vm/compiler/backend/il_x64.cc @@ -4090,7 +4090,9 @@ LocationSummary* BinaryDoubleOpInstr::MakeLocationSummary(Zone* zone, LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall); summary->set_in(0, Location::RequiresFpuRegister()); summary->set_in(1, Location::RequiresFpuRegister()); - summary->set_out(0, Location::SameAsFirstInput()); + summary->set_out(0, Token::IsCommutativeOp(op_kind()) + ? Location::SameAsFirstOrSecondInput() + : Location::SameAsFirstInput()); return summary; } @@ -5934,7 +5936,9 @@ LocationSummary* BinaryInt64OpInstr::MakeLocationSummary(Zone* zone, zone, kNumInputs, kNumTemps, LocationSummary::kNoCall); summary->set_in(0, Location::RequiresRegister()); summary->set_in(1, LocationRegisterOrConstant(right())); - summary->set_out(0, Location::SameAsFirstInput()); + summary->set_out(0, Token::IsCommutativeOp(op_kind()) + ? Location::SameAsFirstOrSecondInput() + : Location::SameAsFirstInput()); return summary; } } @@ -6341,7 +6345,9 @@ LocationSummary* BinaryUint32OpInstr::MakeLocationSummary(Zone* zone, LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall); summary->set_in(0, Location::RequiresRegister()); summary->set_in(1, Location::RequiresRegister()); - summary->set_out(0, Location::SameAsFirstInput()); + summary->set_out(0, Token::IsCommutativeOp(op_kind()) + ? Location::SameAsFirstOrSecondInput() + : Location::SameAsFirstInput()); return summary; } diff --git a/runtime/vm/compiler/backend/linearscan.cc b/runtime/vm/compiler/backend/linearscan.cc index bfd6473372a..ce726957db0 100644 --- a/runtime/vm/compiler/backend/linearscan.cc +++ b/runtime/vm/compiler/backend/linearscan.cc @@ -1338,8 +1338,11 @@ void FlowGraphAllocator::ProcessOneOutput(BlockEntryInstr* block, in_ref->Equals(Location::RequiresFpuRegister())); *out = *in_ref; // Create move that will copy value between input and output. - MoveOperands* move = - AddMoveAt(pos, Location::RequiresRegister(), Location::Any()); + // Inside loops prefer to allocate a register for the value for this + // move, but do not require it. + MoveOperands* move = AddMoveAt( + pos, Location::RequiresRegister(), + block->IsInsideLoop() ? Location::PrefersRegister() : Location::Any()); // Add uses to the live range of the input. LiveRange* input_range = GetLiveRange(input_vreg); @@ -1377,6 +1380,34 @@ void FlowGraphAllocator::ProcessOneOutput(BlockEntryInstr* block, CompleteRange(range, def->RegisterKindForResult()); } +bool FlowGraphAllocator::IsDeadAfterCurrentInstruction(BlockEntryInstr* block, + Instruction* current, + Definition* defn) { + // Do not bother with pair representations for now. + if (defn->HasPairRepresentation()) { + return false; + } + + auto range = GetLiveRange(defn->vreg(0)); + + // Register allocator is building live ranges by visiting blocks in + // postorder and iterating instructions within blocks backwards. When + // we start iterating the block for each value which is live out of the block + // we prepend a use interval covering the whole block to the live range of + // the block. This means all uses which we encounter are being monotonically + // prepended to the start of the range. See |BuildLiveRanges| and |DefineAt| + // for more details. + // + // In other words: it is only possible for a value to have a use *after* the + // current instruction if corresponding range is not empty and it starts + // with a use interval which starts within the current block. It might be + // either interval corresponding to the real use within the block or an + // artificial interval which spans the whole block created for the value + // which flows out of the block. + return range->first_use_interval() == nullptr || + range->first_use_interval()->start() >= block->end_pos(); +} + // Create and update live ranges corresponding to instruction's inputs, // temporaries and output. void FlowGraphAllocator::ProcessOneInstruction(BlockEntryInstr* block, @@ -1436,6 +1467,26 @@ void FlowGraphAllocator::ProcessOneInstruction(BlockEntryInstr* block, } } + if (locs->out(0).IsUnallocated() && + (locs->out(0).policy() == Location::kSameAsFirstOrSecondInput)) { + auto in_left = locs->in(0); + auto in_right = locs->in(1); + // Check if operation has the same constraint on both inputs. + if (in_left.Equals(in_right)) { + // If the first input outlives this instruction but the second does not, + // then we should flip them to reduce register pressure and avoid + // redundant move. + auto defn_left = current->InputAt(0)->definition(); + auto defn_right = current->InputAt(1)->definition(); + if (!IsDeadAfterCurrentInstruction(block, current, defn_left) && + IsDeadAfterCurrentInstruction(block, current, defn_right)) { + current->InputAt(0)->BindTo(defn_right); + current->InputAt(1)->BindTo(defn_left); + } + } + locs->set_out(0, Location::SameAsFirstInput()); + } + const bool output_same_as_first_input = locs->out(0).IsUnallocated() && (locs->out(0).policy() == Location::kSameAsFirstInput); diff --git a/runtime/vm/compiler/backend/linearscan.h b/runtime/vm/compiler/backend/linearscan.h index 7d4195348fa..a42818bc6bb 100644 --- a/runtime/vm/compiler/backend/linearscan.h +++ b/runtime/vm/compiler/backend/linearscan.h @@ -289,6 +289,13 @@ class FlowGraphAllocator : public ValueObject { intptr_t pos, Location::Kind kind); + // Returns true if |defn| is not used after |current|. + // + // Only works during range construction (e.g. ProcessOneInstruction). + bool IsDeadAfterCurrentInstruction(BlockEntryInstr* block, + Instruction* current, + Definition* defn); + void PrintLiveRanges(); // Assign locations for each outgoing argument. Outgoing argumenst are diff --git a/runtime/vm/compiler/backend/linearscan_test.cc b/runtime/vm/compiler/backend/linearscan_test.cc new file mode 100644 index 00000000000..ae5f944fe25 --- /dev/null +++ b/runtime/vm/compiler/backend/linearscan_test.cc @@ -0,0 +1,219 @@ +// Copyright (c) 2024, 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/linearscan.h" + +#include + +#include "vm/compiler/backend/block_builder.h" +#include "vm/compiler/backend/il_printer.h" +#include "vm/compiler/backend/il_test_helper.h" +#include "vm/unit_test.h" +#include "vm/zone_text_buffer.h" + +namespace dart { + +class DummyDef : public Definition { + public: + explicit DummyDef( + Zone* zone, + std::initializer_list> inputs, + Location output, + LocationSummary::ContainsCall contains_call = LocationSummary::kNoCall) + : inputs_(inputs.size()), + summary_(new LocationSummary(zone, + inputs.size(), + /*temp_count=*/0, + contains_call)) { + intptr_t index = 0; + for (auto [defn, loc] : inputs) { + auto v = new Value(defn); + summary_->set_in(index, loc); + v->set_use_index(index); + v->set_instruction(this); + inputs_.Add(v); + index++; + } + summary_->set_out(0, output); + } + + LocationSummary* MakeLocationSummary(Zone* zone, bool opt) const { + return summary_; + } + + virtual void Accept(InstructionVisitor* visitor) { UNREACHABLE(); } + + virtual Tag tag() const { return Instruction::kRedefinition; } + + virtual const char* DebugName() const { return "DummyDef"; } + + virtual intptr_t InputCount() const { return inputs_.length(); } + virtual Value* InputAt(intptr_t i) const { return inputs_[i]; } + + virtual bool MayThrow() const { return false; } + virtual bool ComputeCanDeoptimize() const { return false; } + virtual bool HasUnknownSideEffects() const { return false; } + + private: + virtual void RawSetInputAt(intptr_t i, Value* value) { inputs_[i] = value; } + + GrowableArray inputs_; + LocationSummary* const summary_; + + DISALLOW_COPY_AND_ASSIGN(DummyDef); +}; + +ISOLATE_UNIT_TEST_CASE(LinearScan_TestSameAsFirstOrSecondFlip) { + using compiler::BlockBuilder; + CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true); + FlowGraphBuilderHelper H; + + auto zone = H.flow_graph()->zone(); + + auto b1 = H.flow_graph()->graph_entry()->normal_entry(); + + DummyDef* lhs; + DummyDef* rhs; + DummyDef* binop; + + { + BlockBuilder builder(H.flow_graph(), b1); + + lhs = builder.AddDefinition( + new DummyDef(zone, {}, Location::RequiresRegister())); + rhs = builder.AddDefinition( + new DummyDef(zone, {}, Location::RequiresRegister())); + binop = builder.AddDefinition( + new DummyDef(zone, + {{lhs, Location::RequiresRegister()}, + {rhs, Location::RequiresRegister()}}, + Location::SameAsFirstOrSecondInput())); + // Left hand side of the binary operation is still needed after it. + builder.AddInstruction( + new DummyDef(zone, {{lhs, Location::RequiresRegister()}}, Location())); + builder.AddInstruction(new DartReturnInstr( + InstructionSource(), new Value(binop), S.GetNextDeoptId())); + } + H.FinishGraph(); + + FlowGraphPrinter::PrintGraph("before regalloc", H.flow_graph()); + + H.flow_graph()->InsertMoveArguments(); + // Ensure loop hierarchy has been computed. + H.flow_graph()->GetLoopHierarchy(); + // Perform register allocation on the SSA graph. + FlowGraphAllocator allocator(*H.flow_graph()); + allocator.AllocateRegisters(); + + // There should be no parallel move between binop and rhs and inputs + // to binop should be flipped. + EXPECT_PROPERTY(binop->previous(), + &it == rhs || (it.IsParallelMove() && + it.AsParallelMove()->IsRedundant() && + it.previous() == rhs)); + EXPECT_PROPERTY(binop->InputAt(0)->definition(), &it == rhs); + EXPECT_PROPERTY(binop->InputAt(1)->definition(), &it == lhs); +} + +ISOLATE_UNIT_TEST_CASE(LinearScan_TestSameAsFirstOrSecondNoFlip) { + using compiler::BlockBuilder; + CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true); + FlowGraphBuilderHelper H; + + auto zone = H.flow_graph()->zone(); + + auto b1 = H.flow_graph()->graph_entry()->normal_entry(); + + DummyDef* lhs; + DummyDef* rhs; + DummyDef* binop; + + { + BlockBuilder builder(H.flow_graph(), b1); + + lhs = builder.AddDefinition( + new DummyDef(zone, {}, Location::RequiresRegister())); + rhs = builder.AddDefinition( + new DummyDef(zone, {}, Location::RequiresRegister())); + binop = builder.AddDefinition( + new DummyDef(zone, + {{lhs, Location::RequiresRegister()}, + {rhs, Location::RequiresRegister()}}, + Location::SameAsFirstOrSecondInput())); + // Right hand side of the binary operation is still needed after it. + builder.AddInstruction( + new DummyDef(zone, {{rhs, Location::RequiresRegister()}}, Location())); + builder.AddInstruction(new DartReturnInstr( + InstructionSource(), new Value(binop), S.GetNextDeoptId())); + } + H.FinishGraph(); + + H.flow_graph()->InsertMoveArguments(); + // Ensure loop hierarchy has been computed. + H.flow_graph()->GetLoopHierarchy(); + // Perform register allocation on the SSA graph. + FlowGraphAllocator allocator(*H.flow_graph()); + allocator.AllocateRegisters(); + + // There should be no parallel move between binop and rhs and inputs + // to binop should *not* be flipped. + EXPECT_PROPERTY(binop->previous(), + &it == rhs || (it.IsParallelMove() && + it.AsParallelMove()->IsRedundant() && + it.previous() == rhs)); + EXPECT_PROPERTY(binop->InputAt(0)->definition(), &it == lhs); + EXPECT_PROPERTY(binop->InputAt(1)->definition(), &it == rhs); +} + +ISOLATE_UNIT_TEST_CASE(LinearScan_TestSameAsFirstOrSecondNoFlip2) { + using compiler::BlockBuilder; + CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true); + FlowGraphBuilderHelper H; + + auto zone = H.flow_graph()->zone(); + + auto b1 = H.flow_graph()->graph_entry()->normal_entry(); + + DummyDef* lhs; + DummyDef* rhs; + DummyDef* binop; + + { + BlockBuilder builder(H.flow_graph(), b1); + + lhs = builder.AddDefinition( + new DummyDef(zone, {}, Location::RequiresRegister())); + rhs = builder.AddDefinition( + new DummyDef(zone, {}, Location::RequiresRegister())); + binop = builder.AddDefinition( + new DummyDef(zone, + {{lhs, Location::RequiresRegister()}, + {rhs, Location::RequiresRegister()}}, + Location::SameAsFirstOrSecondInput())); + // Both right and left hand sides of the binary operation are still needed + // after it. + builder.AddInstruction( + new DummyDef(zone, {{rhs, Location::RequiresRegister()}}, Location())); + builder.AddInstruction( + new DummyDef(zone, {{lhs, Location::RequiresRegister()}}, Location())); + builder.AddInstruction(new DartReturnInstr( + InstructionSource(), new Value(binop), S.GetNextDeoptId())); + } + H.FinishGraph(); + + H.flow_graph()->InsertMoveArguments(); + // Ensure loop hierarchy has been computed. + H.flow_graph()->GetLoopHierarchy(); + // Perform register allocation on the SSA graph. + FlowGraphAllocator allocator(*H.flow_graph()); + allocator.AllocateRegisters(); + + // There should be a parallel move between binop and rhs and inputs + // to binop should *not* be flipped. + EXPECT_PROPERTY(binop->previous(), it.IsParallelMove()); + EXPECT_PROPERTY(binop->InputAt(0)->definition(), &it == lhs); + EXPECT_PROPERTY(binop->InputAt(1)->definition(), &it == rhs); +} + +} // namespace dart diff --git a/runtime/vm/compiler/backend/locations.cc b/runtime/vm/compiler/backend/locations.cc index 01eae941e31..792c1fcd86c 100644 --- a/runtime/vm/compiler/backend/locations.cc +++ b/runtime/vm/compiler/backend/locations.cc @@ -401,6 +401,8 @@ const char* Location::Name() const { return "WR"; case kSameAsFirstInput: return "0"; + case kSameAsFirstOrSecondInput: + return "0|1"; case kRequiresStack: return "RS"; } diff --git a/runtime/vm/compiler/backend/locations.h b/runtime/vm/compiler/backend/locations.h index 842cc7cc3d8..c1d2770d81f 100644 --- a/runtime/vm/compiler/backend/locations.h +++ b/runtime/vm/compiler/backend/locations.h @@ -325,6 +325,7 @@ class Location : public ValueObject { kRequiresFpuRegister, kWritableRegister, kSameAsFirstInput, + kSameAsFirstOrSecondInput, // Forces the location to be spilled to the stack. // Currently only used for `Handle` arguments in `FfiCall` instructions. // Only available in optimized mode. @@ -378,6 +379,14 @@ class Location : public ValueObject { return UnallocatedLocation(kSameAsFirstInput); } + // Used for output of a symetric binary operation which have to + // destroy one its inputs (e.g. consider two address arithmetic + // operations live `add`). If any of the inputs is the last use + // of the value then it is cheap to destroy. + static Location SameAsFirstOrSecondInput() { + return UnallocatedLocation(kSameAsFirstOrSecondInput); + } + // Empty location. Used if there the location should be ignored. static Location NoLocation() { return Location(); } diff --git a/runtime/vm/compiler/compiler_sources.gni b/runtime/vm/compiler/compiler_sources.gni index a1d9746ed23..d043b370deb 100644 --- a/runtime/vm/compiler/compiler_sources.gni +++ b/runtime/vm/compiler/compiler_sources.gni @@ -172,6 +172,7 @@ compiler_sources_tests = [ "backend/il_test_helper.h", "backend/il_test_helper.cc", "backend/inliner_test.cc", + "backend/linearscan_test.cc", "backend/locations_helpers_test.cc", "backend/loops_test.cc", "backend/memory_copy_test.cc", diff --git a/runtime/vm/token.h b/runtime/vm/token.h index a670d007d32..a8c481c0e05 100644 --- a/runtime/vm/token.h +++ b/runtime/vm/token.h @@ -225,6 +225,23 @@ class Token { static const Kind kLastKeyword = kWITH; static constexpr int kNumKeywords = kLastKeyword - kFirstKeyword + 1; + static bool IsCommutativeOp(Kind op) { + switch (op) { + case Token::kMUL: + FALL_THROUGH; + case Token::kADD: + FALL_THROUGH; + case Token::kBIT_AND: + FALL_THROUGH; + case Token::kBIT_OR: + FALL_THROUGH; + case Token::kBIT_XOR: + return true; + default: + return false; + } + } + static bool IsAssignmentOperator(Kind tok) { return kASSIGN <= tok && tok <= kASSIGN_COND; }