Reland "[vm/concurrency] When generating Store/Load instructions use guard information that is kept on a slot."

This reverts commit fd31242f2b as the
issue that caused revert should be fixed by removal of boxing heuristic
removed in 8159c38d65.

Fixes https://github.com/dart-lang/sdk/issues/47314
TEST=ci,g3

Change-Id: I42f3f0c6d25639fb326e74d96a51c4c6c0f88f78
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/221601
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Alexander Aprelev <aam@google.com>

runtime/vm/compiler/backend/flow_graph.cc

@@ -132,20 +132,6 @@ Representation FlowGraph::ReturnRepresentationOf(const Function& function) {
   }
 }
 
-Representation FlowGraph::UnboxedFieldRepresentationOf(const Field& field) {
-  switch (field.UnboxedFieldCid()) {
-    case kDoubleCid:
-      return kUnboxedDouble;
-    case kFloat32x4Cid:
-      return kUnboxedFloat32x4;
-    case kFloat64x2Cid:
-      return kUnboxedFloat64x2;
-    default:
-      RELEASE_ASSERT(field.is_non_nullable_integer());
-      return kUnboxedInt64;
-  }
-}
-
 void FlowGraph::ReplaceCurrentInstruction(ForwardInstructionIterator* iterator,
                                           Instruction* current,
                                           Instruction* replacement) {

runtime/vm/compiler/backend/flow_graph.h

@@ -157,8 +157,6 @@ class FlowGraph : public ZoneAllocated {
 
   static Representation ReturnRepresentationOf(const Function& function);
 
-  static Representation UnboxedFieldRepresentationOf(const Field& field);
-
   // The number of variables (or boxes) inside the functions frame - meaning
   // below the frame pointer.  This does not include the expression stack.
   intptr_t num_stack_locals() const {

runtime/vm/compiler/backend/flow_graph_compiler.cc

@@ -203,30 +203,6 @@ FlowGraphCompiler::FlowGraphCompiler(
   ArchSpecificInitialization();
 }
 
-bool FlowGraphCompiler::IsUnboxedField(const Field& field) {
-  // The `field.is_non_nullable_integer()` is set in the kernel loader and can
-  // only be set if we consume a AOT kernel (annotated with inferred types).
-  ASSERT(!field.is_non_nullable_integer() || FLAG_precompiled_mode);
-  const bool valid_class =
-      ((SupportsUnboxedDoubles() && (field.guarded_cid() == kDoubleCid)) ||
-       (SupportsUnboxedSimd128() && (field.guarded_cid() == kFloat32x4Cid)) ||
-       (SupportsUnboxedSimd128() && (field.guarded_cid() == kFloat64x2Cid)) ||
-       field.is_non_nullable_integer());
-  return field.is_unboxing_candidate() && !field.is_nullable() && valid_class;
-}
-
-bool FlowGraphCompiler::IsPotentialUnboxedField(const Field& field) {
-  if (FLAG_precompiled_mode) {
-    // kernel_loader.cc:ReadInferredType sets the guarded cid for fields based
-    // on inferred types from TFA (if available). The guarded cid is therefore
-    // proven to be correct.
-    return IsUnboxedField(field);
-  }
-  return field.is_unboxing_candidate() &&
-         (FlowGraphCompiler::IsUnboxedField(field) ||
-          (field.guarded_cid() == kIllegalCid));
-}
-
 void FlowGraphCompiler::InitCompiler() {
   compressed_stackmaps_builder_ =
       new (zone()) CompressedStackMapsBuilder(zone());

runtime/vm/compiler/backend/flow_graph_compiler.h

@@ -465,9 +465,6 @@ class FlowGraphCompiler : public ValueObject {
   static bool SupportsHardwareDivision();
   static bool CanConvertInt64ToDouble();
 
-  static bool IsUnboxedField(const Field& field);
-  static bool IsPotentialUnboxedField(const Field& field);
-
   // Accessors.
   compiler::Assembler* assembler() const { return assembler_; }
   const ParsedFunction& parsed_function() const { return parsed_function_; }

runtime/vm/compiler/backend/il.cc

@@ -894,17 +894,17 @@ intptr_t CheckClassInstr::ComputeCidMask() const {
 
 bool LoadFieldInstr::IsUnboxedDartFieldLoad() const {
   return slot().representation() == kTagged && slot().IsDartField() &&
-         FlowGraphCompiler::IsUnboxedField(slot().field());
+         slot().IsUnboxed();
 }
 
 bool LoadFieldInstr::IsPotentialUnboxedDartFieldLoad() const {
   return slot().representation() == kTagged && slot().IsDartField() &&
-         FlowGraphCompiler::IsPotentialUnboxedField(slot().field());
+         slot().IsPotentialUnboxed();
 }
 
 Representation LoadFieldInstr::representation() const {
   if (IsUnboxedDartFieldLoad() && CompilerState::Current().is_optimizing()) {
-    return FlowGraph::UnboxedFieldRepresentationOf(slot().field());
+    return slot().UnboxedRepresentation();
   }
   return slot().representation();
 }
@@ -963,12 +963,12 @@ void AllocateTypedDataInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
 
 bool StoreInstanceFieldInstr::IsUnboxedDartFieldStore() const {
   return slot().representation() == kTagged && slot().IsDartField() &&
-         FlowGraphCompiler::IsUnboxedField(slot().field());
+         slot().IsUnboxed();
 }
 
 bool StoreInstanceFieldInstr::IsPotentialUnboxedDartFieldStore() const {
   return slot().representation() == kTagged && slot().IsDartField() &&
-         FlowGraphCompiler::IsPotentialUnboxedField(slot().field());
+         slot().IsPotentialUnboxed();
 }
 
 Representation StoreInstanceFieldInstr::RequiredInputRepresentation(
@@ -979,7 +979,7 @@ Representation StoreInstanceFieldInstr::RequiredInputRepresentation(
     return kTagged;
   }
   if (IsUnboxedDartFieldStore() && CompilerState::Current().is_optimizing()) {
-    return FlowGraph::UnboxedFieldRepresentationOf(slot().field());
+    return slot().UnboxedRepresentation();
   }
   return slot().representation();
 }
@@ -6181,7 +6181,7 @@ Representation StoreIndexedInstr::RequiredInputRepresentation(
 }
 
 bool Utf8ScanInstr::IsScanFlagsUnboxed() const {
-  return FlowGraphCompiler::IsUnboxedField(scan_flags_field_.field());
+  return scan_flags_field_.IsUnboxed();
 }
 
 InvokeMathCFunctionInstr::InvokeMathCFunctionInstr(

runtime/vm/compiler/backend/slot.cc

@@ -331,6 +331,58 @@ const Slot& Slot::GetArrayElementSlot(Thread* thread,
   return SlotCache::Instance(thread).Canonicalize(slot);
 }
 
+FieldGuardState::FieldGuardState(const Field& field)
+    : state_(GuardedCidBits::encode(field.guarded_cid()) |
+             IsNonNullableIntegerBit::encode(field.is_non_nullable_integer()) |
+             IsUnboxingCandidateBit::encode(field.is_unboxing_candidate()) |
+             IsNullableBit::encode(field.is_nullable())) {}
+
+bool FieldGuardState::IsUnboxed() const {
+  ASSERT(!is_non_nullable_integer() || FLAG_precompiled_mode);
+  const bool valid_class = ((FlowGraphCompiler::SupportsUnboxedDoubles() &&
+                             (guarded_cid() == kDoubleCid)) ||
+                            (FlowGraphCompiler::SupportsUnboxedSimd128() &&
+                             (guarded_cid() == kFloat32x4Cid)) ||
+                            (FlowGraphCompiler::SupportsUnboxedSimd128() &&
+                             (guarded_cid() == kFloat64x2Cid)) ||
+                            is_non_nullable_integer());
+  return is_unboxing_candidate() && !is_nullable() && valid_class;
+}
+
+bool FieldGuardState::IsPotentialUnboxed() const {
+  if (FLAG_precompiled_mode) {
+    // kernel_loader.cc:ReadInferredType sets the guarded cid for fields based
+    // on inferred types from TFA (if available). The guarded cid is therefore
+    // proven to be correct.
+    return IsUnboxed();
+  }
+
+  return is_unboxing_candidate() &&
+         (IsUnboxed() || (guarded_cid() == kIllegalCid));
+}
+
+bool Slot::IsUnboxed() const {
+  return field_guard_state().IsUnboxed();
+}
+
+bool Slot::IsPotentialUnboxed() const {
+  return field_guard_state().IsPotentialUnboxed();
+}
+
+Representation Slot::UnboxedRepresentation() const {
+  switch (field_guard_state().guarded_cid()) {
+    case kDoubleCid:
+      return kUnboxedDouble;
+    case kFloat32x4Cid:
+      return kUnboxedFloat32x4;
+    case kFloat64x2Cid:
+      return kUnboxedFloat64x2;
+    default:
+      RELEASE_ASSERT(field_guard_state().is_non_nullable_integer());
+      return kUnboxedInt64;
+  }
+}
+
 const Slot& Slot::Get(const Field& field,
                       const ParsedFunction* parsed_function) {
   Thread* thread = Thread::Current();
@@ -354,16 +406,18 @@ const Slot& Slot::Get(const Field& field,
     is_nullable = false;
   }
 
+  FieldGuardState field_guard_state(field);
+
   bool used_guarded_state = false;
-  if (field.guarded_cid() != kIllegalCid &&
-      field.guarded_cid() != kDynamicCid) {
+  if (field_guard_state.guarded_cid() != kIllegalCid &&
+      field_guard_state.guarded_cid() != kDynamicCid) {
     // Use guarded state if it is more precise then what we already have.
     if (nullable_cid == kDynamicCid) {
-      nullable_cid = field.guarded_cid();
+      nullable_cid = field_guard_state.guarded_cid();
       used_guarded_state = true;
     }
 
-    if (is_nullable && !field.is_nullable()) {
+    if (is_nullable && !field_guard_state.is_nullable()) {
       is_nullable = false;
       used_guarded_state = true;
     }
@@ -377,10 +431,10 @@ const Slot& Slot::Get(const Field& field,
     used_guarded_state = false;
   }
 
-  if (field.is_non_nullable_integer()) {
+  if (field_guard_state.is_non_nullable_integer()) {
     ASSERT(FLAG_precompiled_mode);
     is_nullable = false;
-    if (FlowGraphCompiler::IsUnboxedField(field)) {
+    if (field_guard_state.IsUnboxed()) {
       rep = kUnboxedInt64;
     }
   }
@@ -397,7 +451,7 @@ const Slot& Slot::Get(const Field& field,
           IsSentinelVisibleBit::encode(field.is_late() && field.is_final() &&
                                        !field.has_initializer()),
       nullable_cid, compiler::target::Field::OffsetOf(field), &field, &type,
-      rep));
+      rep, field_guard_state));
 
   // If properties of this slot were based on the guarded state make sure
   // to add the field to the list of guarded fields. Note that during background

runtime/vm/compiler/backend/slot.h

@@ -166,6 +166,35 @@ class ParsedFunction;
   NONNULLABLE_BOXED_NATIVE_SLOTS_LIST(V)                                       \
   UNBOXED_NATIVE_SLOTS_LIST(V)
 
+class FieldGuardState {
+ public:
+  FieldGuardState() : state_(0) {}
+  explicit FieldGuardState(const Field& field);
+
+  intptr_t guarded_cid() const { return GuardedCidBits::decode(state_); }
+  bool is_non_nullable_integer() const {
+    return IsNonNullableIntegerBit::decode(state_);
+  }
+  bool is_unboxing_candidate() const {
+    return IsUnboxingCandidateBit::decode(state_);
+  }
+  bool is_nullable() const { return IsNullableBit::decode(state_); }
+
+  bool IsUnboxed() const;
+  bool IsPotentialUnboxed() const;
+
+ private:
+  using GuardedCidBits = BitField<int32_t, ClassIdTagType, 0, 16>;
+  using IsNonNullableIntegerBit =
+      BitField<int32_t, bool, GuardedCidBits::kNextBit, 1>;
+  using IsUnboxingCandidateBit =
+      BitField<int32_t, bool, IsNonNullableIntegerBit::kNextBit, 1>;
+  using IsNullableBit =
+      BitField<int32_t, bool, IsUnboxingCandidateBit::kNextBit, 1>;
+
+  const int32_t state_;
+};
+
 // Slot is an abstraction that describes an readable (and possibly writeable)
 // location within an object.
 //
@@ -298,6 +327,10 @@ class Slot : public ZoneAllocated {
     return kind() == Kind::kCapturedVariable || kind() == Kind::kContext_parent;
   }
 
+  bool IsUnboxed() const;
+  bool IsPotentialUnboxed() const;
+  Representation UnboxedRepresentation() const;
+
  private:
   friend class FlowGraphDeserializer;  // For GetNativeSlot.
 
@@ -307,12 +340,14 @@ class Slot : public ZoneAllocated {
        intptr_t offset_in_bytes,
        const void* data,
        const AbstractType* static_type,
-       Representation representation)
+       Representation representation,
+       const FieldGuardState& field_guard_state = FieldGuardState())
       : kind_(kind),
         flags_(bits),
         cid_(cid),
         offset_in_bytes_(offset_in_bytes),
         representation_(representation),
+        field_guard_state_(field_guard_state),
         data_(data),
         static_type_(static_type) {}
 
@@ -323,7 +358,8 @@ class Slot : public ZoneAllocated {
              other.offset_in_bytes_,
              other.data_,
              other.static_type_,
-             other.representation_) {}
+             other.representation_,
+             other.field_guard_state_) {}
 
   using IsImmutableBit = BitField<int8_t, bool, 0, 1>;
   using IsNullableBit = BitField<int8_t, bool, IsImmutableBit::kNextBit, 1>;
@@ -342,6 +378,10 @@ class Slot : public ZoneAllocated {
   static AcqRelAtomic<Slot*> native_fields_;
   static const Slot& GetNativeSlot(Kind kind);
 
+  const FieldGuardState& field_guard_state() const {
+    return field_guard_state_;
+  }
+
   const Kind kind_;
   const int8_t flags_;        // is_immutable, is_nullable
   const ClassIdTagType cid_;  // Concrete cid of a value or kDynamicCid.
@@ -349,6 +389,8 @@ class Slot : public ZoneAllocated {
   const intptr_t offset_in_bytes_;
   const Representation representation_;
 
+  const FieldGuardState field_guard_state_;
+
   // Kind dependent data:
   //   - name as a Dart String object for local variables;
   //   - name as a C string for native slots;

runtime/vm/compiler/graph_intrinsifier.cc

@@ -1050,7 +1050,8 @@ bool GraphIntrinsifier::Build_ImplicitGetter(FlowGraph* flow_graph) {
   // [Intrinsifier::CanIntrinsifyFieldAccessor])
   auto zone = flow_graph->zone();
   const auto& function = flow_graph->function();
-  ASSERT(Intrinsifier::CanIntrinsifyFieldAccessor(function));
+  ASSERT(
+      Intrinsifier::CanIntrinsifyFieldAccessor(flow_graph->parsed_function()));
 
   auto& field = Field::Handle(zone, function.accessor_field());
   if (CompilerState::Current().should_clone_fields()) {
@@ -1072,12 +1073,10 @@ bool GraphIntrinsifier::Build_ImplicitGetter(FlowGraph* flow_graph) {
 
   // We only support cases where we do not have to create a box (whose
   // allocation could fail).
-  ASSERT(function.HasUnboxedReturnValue() ||
-         !FlowGraphCompiler::IsUnboxedField(field));
+  ASSERT(function.HasUnboxedReturnValue() || !slot.IsUnboxed());
 
   // We might need to unbox the field value before returning.
-  if (function.HasUnboxedReturnValue() &&
-      !FlowGraphCompiler::IsUnboxedField(field)) {
+  if (function.HasUnboxedReturnValue() && !slot.IsUnboxed()) {
     ASSERT(FLAG_precompiled_mode);
     field_value = builder.AddUnboxInstr(
         FlowGraph::ReturnRepresentationOf(flow_graph->function()),
@@ -1093,21 +1092,19 @@ bool GraphIntrinsifier::Build_ImplicitSetter(FlowGraph* flow_graph) {
   // [Intrinsifier::CanIntrinsifyFieldAccessor])
   auto zone = flow_graph->zone();
   const auto& function = flow_graph->function();
-  ASSERT(Intrinsifier::CanIntrinsifyFieldAccessor(function));
+  ASSERT(
+      Intrinsifier::CanIntrinsifyFieldAccessor(flow_graph->parsed_function()));
 
   auto& field = Field::Handle(zone, function.accessor_field());
   if (CompilerState::Current().should_clone_fields()) {
     field = field.CloneFromOriginal();
   }
   ASSERT(field.is_instance() && !field.is_final());
-  ASSERT(!function.HasUnboxedParameters() ||
-         FlowGraphCompiler::IsUnboxedField(field));
-
   const auto& slot = Slot::Get(field, &flow_graph->parsed_function());
+  ASSERT(!function.HasUnboxedParameters() || slot.IsUnboxed());
 
-  const auto barrier_mode = FlowGraphCompiler::IsUnboxedField(field)
-                                ? kNoStoreBarrier
-                                : kEmitStoreBarrier;
+  const auto barrier_mode =
+      slot.IsUnboxed() ? kNoStoreBarrier : kEmitStoreBarrier;
 
   flow_graph->CreateCommonConstants();
   GraphEntryInstr* graph_entry = flow_graph->graph_entry();
@@ -1118,14 +1115,13 @@ bool GraphIntrinsifier::Build_ImplicitSetter(FlowGraph* flow_graph) {
   auto value = builder.AddParameter(1, /*with_frame=*/false);
   VerifyParameterIsBoxed(&builder, 0);
 
-  if (!function.HasUnboxedParameters() &&
-      FlowGraphCompiler::IsUnboxedField(field)) {
+  if (!function.HasUnboxedParameters() && slot.IsUnboxed()) {
     // We do not support storing to possibly guarded fields in JIT in graph
     // intrinsics.
     ASSERT(FLAG_precompiled_mode);
-    value = builder.AddUnboxInstr(
-        FlowGraph::UnboxedFieldRepresentationOf(field), new Value(value),
-        /*is_checked=*/true);
+    value =
+        builder.AddUnboxInstr(slot.UnboxedRepresentation(), new Value(value),
+                              /*is_checked=*/true);
   }
 
   builder.AddInstruction(new (zone) StoreInstanceFieldInstr(

runtime/vm/compiler/intrinsifier.cc

@@ -25,7 +25,9 @@ DEFINE_FLAG(bool, trace_intrinsifier, false, "Trace intrinsifier");
 
 namespace compiler {
 
-bool Intrinsifier::CanIntrinsify(const Function& function) {
+bool Intrinsifier::CanIntrinsify(const ParsedFunction& parsed_function) {
+  const Function& function = parsed_function.function();
+
   if (FLAG_trace_intrinsifier) {
     THR_Print("CanIntrinsify %s ->", function.ToQualifiedCString());
   }
@@ -45,7 +47,8 @@ bool Intrinsifier::CanIntrinsify(const Function& function) {
     }
     return false;
   }
-  if (!function.is_intrinsic() && !CanIntrinsifyFieldAccessor(function)) {
+  if (!function.is_intrinsic() &&
+      !CanIntrinsifyFieldAccessor(parsed_function)) {
     if (FLAG_trace_intrinsifier) {
       THR_Print("No, not intrinsic function.\n");
     }
@@ -73,7 +76,10 @@ bool Intrinsifier::CanIntrinsify(const Function& function) {
   return true;
 }
 
-bool Intrinsifier::CanIntrinsifyFieldAccessor(const Function& function) {
+bool Intrinsifier::CanIntrinsifyFieldAccessor(
+    const ParsedFunction& parsed_function) {
+  const Function& function = parsed_function.function();
+
   const bool is_getter = function.IsImplicitGetterFunction();
   const bool is_setter = function.IsImplicitSetterFunction();
   if (!is_getter && !is_setter) return false;
@@ -97,11 +103,13 @@ bool Intrinsifier::CanIntrinsifyFieldAccessor(const Function& function) {
   // We only graph intrinsify implicit instance getters/setter for now.
   if (!field.is_instance()) return false;
 
+  const auto& slot = Slot::Get(field, &parsed_function);
+
   if (is_getter) {
     // We don't support complex getter cases.
     if (field.is_late() || field.needs_load_guard()) return false;
 
-    if (FlowGraphCompiler::IsPotentialUnboxedField(field)) {
+    if (slot.IsPotentialUnboxed()) {
       if (function.HasUnboxedReturnValue()) {
         // In AOT mode: Unboxed fields contain the unboxed value and can be
         // returned in unboxed form.
@@ -136,7 +144,7 @@ bool Intrinsifier::CanIntrinsifyFieldAccessor(const Function& function) {
     // avoid the need for boxing (which we cannot do in the intrinsic).
     if (function.HasUnboxedParameters()) {
       ASSERT(FLAG_precompiled_mode);
-      if (!FlowGraphCompiler::IsUnboxedField(field)) {
+      if (!slot.IsUnboxed()) {
         return false;
       }
     }
@@ -253,8 +261,7 @@ void Intrinsifier::InitializeState() {
 // Returns true if fall-through code can be omitted.
 bool Intrinsifier::Intrinsify(const ParsedFunction& parsed_function,
                               FlowGraphCompiler* compiler) {
-  const Function& function = parsed_function.function();
-  if (!CanIntrinsify(function)) {
+  if (!CanIntrinsify(parsed_function)) {
     return false;
   }
 
@@ -262,6 +269,7 @@ bool Intrinsifier::Intrinsify(const ParsedFunction& parsed_function,
     return compiler->intrinsic_slow_path_label()->IsUnused();
   }
 
+  const Function& function = parsed_function.function();
 #if !defined(HASH_IN_OBJECT_HEADER)
   // These two are more complicated on 32 bit platforms, where the
   // identity hash is not stored in the header of the object.  We

runtime/vm/compiler/intrinsifier.h

@@ -35,8 +35,8 @@ class Intrinsifier : public AllStatic {
 
  private:
   friend class GraphIntrinsifier;  // For CanIntrinsifyFieldAccessor.
-  static bool CanIntrinsify(const Function& function);
-  static bool CanIntrinsifyFieldAccessor(const Function& function);
+  static bool CanIntrinsify(const ParsedFunction& parsed_function);
+  static bool CanIntrinsifyFieldAccessor(const ParsedFunction& parsed_function);
 };
 
 }  // namespace compiler