[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 <alexmarkov@google.com>
Commit-Queue: Slava Egorov <vegorov@google.com>

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:

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);

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;

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;
 }
 

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);

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

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 <utility>
+
+#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<std::pair<Definition*, Location>> 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<Value*> 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

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";
       }

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(); }
 

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",

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;
   }