Revert "[vm/compiler] Mark more stores as initializing."

This reverts commit eb87e79.

Reason for revert: Multiple configurations are red

IRTest_InitializingStores:

https://ci.chromium.org/p/dart/builders/ci.sandbox/vm-dartkb-linux-debug-simarm64/1684
https://ci.chromium.org/p/dart/builders/ci.sandbox/vm-dartkb-linux-product-simarm64/1796

http_server_test:
https://ci.chromium.org/p/dart/builders/ci.sandbox/vm-kernel-precomp-linux-release-simarm/6796


Original change's description:
> [vm/compiler] Mark more stores as initializing.
> 
> We inline allocation of contexts and closures manually so stores
> which are effectively initializing were not marked as such.
> 
> Additionally permit elimination of initializing stores into context
> objects in AOT mode because all contexts are null-initialized there.
> 
> Bug: #38454
> Change-Id: I27886d85160b8600e6e0d38d7bf389d20006fa44
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/121849
> Commit-Queue: Vyacheslav Egorov <[email protected]>
> Reviewed-by: Martin Kustermann <[email protected]>

[email protected],[email protected]

Change-Id: I4c2df7b675fa3cdab22a8708440dc45ff3ec3476
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: #38454
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/121885
Reviewed-by: Ben Konyi <[email protected]>
Commit-Queue: Ben Konyi <[email protected]>

Author: bkonyi

runtime/vm/compiler/backend/il.cc

@@ -903,16 +903,6 @@ Representation LoadFieldInstr::representation() const {
   return kTagged;
 }
 
-AllocateUninitializedContextInstr::AllocateUninitializedContextInstr(
-    TokenPosition token_pos,
-    intptr_t num_context_variables)
-    : token_pos_(token_pos),
-      num_context_variables_(num_context_variables),
-      identity_(AliasIdentity::Unknown()) {
-  // This instruction is not used in AOT for code size reasons.
-  ASSERT(!FLAG_precompiled_mode);
-}
-
 bool StoreInstanceFieldInstr::IsUnboxedStore() const {
   return FLAG_unbox_numeric_fields && slot().IsDartField() &&
          FlowGraphCompiler::IsUnboxedField(slot().field());
@@ -945,12 +935,11 @@ Representation StoreInstanceFieldInstr::RequiredInputRepresentation(
 Instruction* StoreInstanceFieldInstr::Canonicalize(FlowGraph* flow_graph) {
   // Dart objects are allocated null-initialized, which means we can eliminate
   // all initializing stores which store null value.
-  // Context objects can be allocated uninitialized as a performance
-  // optimization in JIT mode - however in AOT mode we always allocate them
-  // null initialized.
-  if (is_initialization_ &&
-      (!slot().IsContextSlot() ||
-       !instance()->definition()->IsAllocateUninitializedContext()) &&
+  // TODO(dartbug.com/38454) Context objects can be allocated uninitialized
+  // as a performance optimization (all initializing stores are inlined into
+  // the caller, which allocates the context). Investigate if this can be
+  // changed to align with normal Dart objects for code size reasons.
+  if (is_initialization_ && slot().IsDartField() &&
       value()->BindsToConstantNull()) {
     return nullptr;
   }

runtime/vm/compiler/backend/il.h

@@ -5332,7 +5332,10 @@ class AllocateUninitializedContextInstr
     : public TemplateAllocation<0, NoThrow> {
  public:
   AllocateUninitializedContextInstr(TokenPosition token_pos,
-                                    intptr_t num_context_variables);
+                                    intptr_t num_context_variables)
+      : token_pos_(token_pos),
+        num_context_variables_(num_context_variables),
+        identity_(AliasIdentity::Unknown()) {}
 
   DECLARE_INSTRUCTION(AllocateUninitializedContext)
   virtual CompileType ComputeType() const;

runtime/vm/compiler/backend/il_test.cc

@@ -3,9 +3,6 @@
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/compiler/backend/il.h"
-
-#include <vector>
-
 #include "vm/compiler/backend/il_test_helper.h"
 #include "vm/unit_test.h"
 
@@ -88,94 +85,4 @@ ISOLATE_UNIT_TEST_CASE(IRTest_EliminateWriteBarrier) {
   EXPECT(!store_indexed->value()->NeedsWriteBarrier());
 }
 
-static void ExpectStores(FlowGraph* flow_graph,
-                         const std::vector<const char*>& expected_stores) {
-  size_t next_expected_store = 0;
-  for (BlockIterator block_it = flow_graph->reverse_postorder_iterator();
-       !block_it.Done(); block_it.Advance()) {
-    for (ForwardInstructionIterator it(block_it.Current()); !it.Done();
-         it.Advance()) {
-      if (auto store = it.Current()->AsStoreInstanceField()) {
-        EXPECT_LT(next_expected_store, expected_stores.size());
-        EXPECT_STREQ(expected_stores[next_expected_store],
-                     store->slot().Name());
-        next_expected_store++;
-      }
-    }
-  }
-}
-
-static void RunInitializingStoresTest(
-    const Library& root_library,
-    const char* function_name,
-    CompilerPass::PipelineMode mode,
-    const std::vector<const char*>& expected_stores) {
-  const auto& function =
-      Function::Handle(GetFunction(root_library, function_name));
-  TestPipeline pipeline(function, mode);
-  FlowGraph* flow_graph = pipeline.RunPasses({
-      CompilerPass::kComputeSSA,
-      CompilerPass::kTypePropagation,
-      CompilerPass::kApplyICData,
-      CompilerPass::kInlining,
-      CompilerPass::kTypePropagation,
-      CompilerPass::kSelectRepresentations,
-      CompilerPass::kCanonicalize,
-      CompilerPass::kConstantPropagation,
-  });
-  ASSERT(flow_graph != nullptr);
-  ExpectStores(flow_graph, expected_stores);
-}
-
-ISOLATE_UNIT_TEST_CASE(IRTest_InitializingStores) {
-  const char* kScript = R"(
-    class Bar {
-      var f;
-      var g;
-
-      Bar({this.f, this.g});
-    }
-    Bar f1() => Bar(f: 10);
-    Bar f2() => Bar(g: 10);
-    f3() {
-      return () { };
-    }
-    f4<T>({T value}) {
-      return () { return value; };
-    }
-    main() {
-      f1();
-      f2();
-      f3();
-      f4();
-    }
-  )";
-  const auto& root_library = Library::Handle(LoadTestScript(kScript));
-  Invoke(root_library, "main");
-
-  RunInitializingStoresTest(root_library, "f1", CompilerPass::kJIT,
-                            /*expected_stores=*/{"f"});
-  RunInitializingStoresTest(root_library, "f2", CompilerPass::kJIT,
-                            /*expected_stores=*/{"g"});
-  RunInitializingStoresTest(
-      root_library, "f3", CompilerPass::kJIT,
-      /*expected_stores=*/
-      {"Closure.delayed_type_arguments", "Closure.function"});
-
-  // Note that in JIT mode we lower context allocation in a way that hinders
-  // removal of initializing moves so there would be some redundant stores of
-  // null left in the graph. In AOT mode we don't apply this optimization
-  // which enables us to remove more stores.
-  RunInitializingStoresTest(
-      root_library, "f4", CompilerPass::kJIT, /*expected_stores=*/
-      {"value", "Context.parent", "Context.parent", "value",
-       "Closure.function_type_arguments", "Closure.delayed_type_arguments",
-       "Closure.function", "Closure.context"});
-  RunInitializingStoresTest(root_library, "f4",
-                            CompilerPass::kAOT, /*expected_stores=*/
-                            {"value", "Closure.function_type_arguments",
-                             "Closure.delayed_type_arguments",
-                             "Closure.function", "Closure.context"});
-}
-
 }  // namespace dart

runtime/vm/compiler/backend/slot.h

@@ -181,10 +181,6 @@ class Slot : public ZoneAllocated {
 
   bool IsIdentical(const Slot& other) const { return this == &other; }
 
-  bool IsContextSlot() const {
-    return kind() == Kind::kCapturedVariable || kind() == Kind::kContext_parent;
-  }
-
  private:
   friend class FlowGraphDeserializer;  // For GetNativeSlot.
 

runtime/vm/compiler/frontend/base_flow_graph_builder.cc

@@ -478,22 +478,19 @@ const Field& BaseFlowGraphBuilder::MayCloneField(const Field& field) {
 Fragment BaseFlowGraphBuilder::StoreInstanceField(
     TokenPosition position,
     const Slot& field,
-    StoreInstanceFieldInstr::Kind
-        kind /* = StoreInstanceFieldInstr::Kind::kOther */,
-    StoreBarrierType emit_store_barrier /* = kEmitStoreBarrier */) {
+    StoreBarrierType emit_store_barrier) {
   Value* value = Pop();
   if (value->BindsToConstant()) {
     emit_store_barrier = kNoStoreBarrier;
   }
   StoreInstanceFieldInstr* store = new (Z) StoreInstanceFieldInstr(
-      field, Pop(), value, emit_store_barrier, position, kind);
+      field, Pop(), value, emit_store_barrier, position);
   return Fragment(store);
 }
 
 Fragment BaseFlowGraphBuilder::StoreInstanceField(
     const Field& field,
-    StoreInstanceFieldInstr::Kind
-        kind /* = StoreInstanceFieldInstr::Kind::kOther */,
+    bool is_initialization_store,
     StoreBarrierType emit_store_barrier) {
   Value* value = Pop();
   if (value->BindsToConstant()) {
@@ -502,15 +499,16 @@ Fragment BaseFlowGraphBuilder::StoreInstanceField(
 
   StoreInstanceFieldInstr* store = new (Z) StoreInstanceFieldInstr(
       MayCloneField(field), Pop(), value, emit_store_barrier,
-      TokenPosition::kNoSource, parsed_function_, kind);
+      TokenPosition::kNoSource, parsed_function_,
+      is_initialization_store ? StoreInstanceFieldInstr::Kind::kInitializing
+                              : StoreInstanceFieldInstr::Kind::kOther);
 
   return Fragment(store);
 }
 
 Fragment BaseFlowGraphBuilder::StoreInstanceFieldGuarded(
     const Field& field,
-    StoreInstanceFieldInstr::Kind
-        kind /* = StoreInstanceFieldInstr::Kind::kOther */) {
+    bool is_initialization_store) {
   Fragment instructions;
   const Field& field_clone = MayCloneField(field);
   if (I->use_field_guards()) {
@@ -539,7 +537,7 @@ Fragment BaseFlowGraphBuilder::StoreInstanceFieldGuarded(
           new (Z) GuardFieldTypeInstr(Pop(), field_clone, GetNextDeoptId());
     }
   }
-  instructions += StoreInstanceField(field_clone, kind);
+  instructions += StoreInstanceField(field_clone, is_initialization_store);
   return instructions;
 }
 
@@ -935,13 +933,12 @@ Fragment BaseFlowGraphBuilder::BuildFfiAsFunctionInternalCall(
 
   code += LoadLocal(closure);
   code += LoadLocal(context);
-  code += StoreInstanceField(TokenPosition::kNoSource, Slot::Closure_context(),
-                             StoreInstanceFieldInstr::Kind::kInitializing);
+  code += StoreInstanceField(TokenPosition::kNoSource, Slot::Closure_context());
 
   code += LoadLocal(closure);
   code += Constant(target);
-  code += StoreInstanceField(TokenPosition::kNoSource, Slot::Closure_function(),
-                             StoreInstanceFieldInstr::Kind::kInitializing);
+  code +=
+      StoreInstanceField(TokenPosition::kNoSource, Slot::Closure_function());
 
   // Drop address and context.
   code += DropTempsPreserveTop(2);

runtime/vm/compiler/frontend/base_flow_graph_builder.h

@@ -186,17 +186,13 @@ class BaseFlowGraphBuilder {
   Fragment StoreInstanceField(
       TokenPosition position,
       const Slot& field,
-      StoreInstanceFieldInstr::Kind kind =
-          StoreInstanceFieldInstr::Kind::kOther,
       StoreBarrierType emit_store_barrier = kEmitStoreBarrier);
   Fragment StoreInstanceField(
       const Field& field,
-      StoreInstanceFieldInstr::Kind kind =
-          StoreInstanceFieldInstr::Kind::kOther,
+      bool is_initialization_store,
       StoreBarrierType emit_store_barrier = kEmitStoreBarrier);
   Fragment StoreInstanceFieldGuarded(const Field& field,
-                                     StoreInstanceFieldInstr::Kind kind =
-                                         StoreInstanceFieldInstr::Kind::kOther);
+                                     bool is_initialization_store);
   Fragment LoadStaticField();
   Fragment RedefinitionWithType(const AbstractType& type);
   Fragment StoreStaticField(TokenPosition position, const Field& field);

runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc

@@ -1218,15 +1218,13 @@ void BytecodeFlowGraphBuilder::BuildStoreFieldTOS() {
 
   if (field.Owner() == isolate()->object_store()->closure_class()) {
     // Stores to _Closure fields are lower-level.
-    code_ +=
-        B->StoreInstanceField(position_, ClosureSlotByField(field),
-                              StoreInstanceFieldInstr::Kind::kInitializing);
+    code_ += B->StoreInstanceField(position_, ClosureSlotByField(field));
   } else {
     // The rest of the StoreFieldTOS are for field initializers.
     // TODO(alexmarkov): Consider adding a flag to StoreFieldTOS or even
     // adding a separate bytecode instruction.
-    code_ += B->StoreInstanceFieldGuarded(
-        field, StoreInstanceFieldInstr::Kind::kInitializing);
+    code_ += B->StoreInstanceFieldGuarded(field,
+                                          /* is_initialization_store = */ true);
   }
 }
 
@@ -1253,8 +1251,7 @@ void BytecodeFlowGraphBuilder::BuildLoadFieldTOS() {
 void BytecodeFlowGraphBuilder::BuildStoreContextParent() {
   LoadStackSlots(2);
 
-  code_ += B->StoreInstanceField(position_, Slot::Context_parent(),
-                                 StoreInstanceFieldInstr::Kind::kInitializing);
+  code_ += B->StoreInstanceField(position_, Slot::Context_parent());
 }
 
 void BytecodeFlowGraphBuilder::BuildLoadContextParent() {

runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc

@@ -187,8 +187,7 @@ Fragment StreamingFlowGraphBuilder::BuildFieldInitializer(
   if (only_for_side_effects) {
     instructions += Drop();
   } else {
-    instructions += flow_graph_builder_->StoreInstanceFieldGuarded(
-        field, StoreInstanceFieldInstr::Kind::kInitializing);
+    instructions += flow_graph_builder_->StoreInstanceFieldGuarded(field, true);
   }
   return instructions;
 }
@@ -675,8 +674,7 @@ Fragment StreamingFlowGraphBuilder::SetupCapturedParameters(
         body += LoadLocal(&raw_parameter);
         body += flow_graph_builder_->StoreInstanceField(
             TokenPosition::kNoSource,
-            Slot::GetContextVariableSlotFor(thread(), *variable),
-            StoreInstanceFieldInstr::Kind::kInitializing);
+            Slot::GetContextVariableSlotFor(thread(), *variable));
         body += NullConstant();
         body += StoreLocal(TokenPosition::kNoSource, &raw_parameter);
         body += Drop();
@@ -3937,8 +3935,7 @@ Fragment StreamingFlowGraphBuilder::BuildPartialTearoffInstantiation(
   instructions += LoadLocal(new_closure);
   instructions += LoadLocal(type_args_vec);
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_delayed_type_arguments(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_delayed_type_arguments());
   instructions += Drop();  // Drop type args.
 
   // Copy over the target function.
@@ -3947,34 +3944,30 @@ Fragment StreamingFlowGraphBuilder::BuildPartialTearoffInstantiation(
   instructions +=
       flow_graph_builder_->LoadNativeField(Slot::Closure_function());
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_function(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_function());
 
   // Copy over the instantiator type arguments.
   instructions += LoadLocal(new_closure);
   instructions += LoadLocal(original_closure);
   instructions += flow_graph_builder_->LoadNativeField(
       Slot::Closure_instantiator_type_arguments());
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_instantiator_type_arguments(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_instantiator_type_arguments());
 
   // Copy over the function type arguments.
   instructions += LoadLocal(new_closure);
   instructions += LoadLocal(original_closure);
   instructions += flow_graph_builder_->LoadNativeField(
       Slot::Closure_function_type_arguments());
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_function_type_arguments(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_function_type_arguments());
 
   // Copy over the context.
   instructions += LoadLocal(new_closure);
   instructions += LoadLocal(original_closure);
   instructions += flow_graph_builder_->LoadNativeField(Slot::Closure_context());
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_context(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_context());
 
   instructions += DropTempsPreserveTop(1);  // Drop old closure.
 
@@ -5096,36 +5089,31 @@ Fragment StreamingFlowGraphBuilder::BuildFunctionNode(
     instructions += LoadLocal(closure);
     instructions += LoadInstantiatorTypeArguments();
     instructions += flow_graph_builder_->StoreInstanceField(
-        TokenPosition::kNoSource, Slot::Closure_instantiator_type_arguments(),
-        StoreInstanceFieldInstr::Kind::kInitializing);
+        TokenPosition::kNoSource, Slot::Closure_instantiator_type_arguments());
   }
 
   // TODO(30455): We only need to save these if the closure uses any captured
   // type parameters.
   instructions += LoadLocal(closure);
   instructions += LoadFunctionTypeArguments();
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_function_type_arguments(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_function_type_arguments());
 
   instructions += LoadLocal(closure);
   instructions += Constant(Object::empty_type_arguments());
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_delayed_type_arguments(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_delayed_type_arguments());
 
   // Store the function and the context in the closure.
   instructions += LoadLocal(closure);
   instructions += Constant(function);
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_function(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_function());
 
   instructions += LoadLocal(closure);
   instructions += LoadLocal(parsed_function()->current_context_var());
   instructions += flow_graph_builder_->StoreInstanceField(
-      TokenPosition::kNoSource, Slot::Closure_context(),
-      StoreInstanceFieldInstr::Kind::kInitializing);
+      TokenPosition::kNoSource, Slot::Closure_context());
 
   return instructions;
 }