[vm] Make String.codeUnitAt monomorphic

The new implementation of String.codeUnitAt is always inlined and
doesn't depend on the polymorphic inlining of recognized methods.

String.codeUnitAt now has a custom body in the flow graph builder
which  performs non-speculative bounds check and then branches between
OneByteString and TwoByteString. Corresponding graph intrinsic and
native method are removed.

This change also fixes passing of unboxed arguments to runtime
in the slow path of GenericCheckBound instruction in JIT mode
(when shared stubs are not used).

TEST=ci

Change-Id: Iab2805fc752df84c37089165f828e31aca5f043f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359000
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>

runtime/lib/string.cc

@@ -448,15 +448,6 @@ DEFINE_NATIVE_ENTRY(String_charAt, 0, 2) {
   return Symbols::FromCharCode(thread, static_cast<int32_t>(value));
 }
 
-// Returns the 16-bit UTF-16 code unit at the given index.
-DEFINE_NATIVE_ENTRY(String_codeUnitAt, 0, 2) {
-  const String& receiver =
-      String::CheckedHandle(zone, arguments->NativeArgAt(0));
-  GET_NON_NULL_NATIVE_ARGUMENT(Integer, index, arguments->NativeArgAt(1));
-  uint16_t value = StringValueAt(receiver, index);
-  return Smi::New(static_cast<intptr_t>(value));
-}
-
 DEFINE_NATIVE_ENTRY(String_concat, 0, 2) {
   const String& receiver =
       String::CheckedHandle(zone, arguments->NativeArgAt(0));

runtime/vm/bootstrap_natives.h

@@ -135,7 +135,6 @@ namespace dart {
   V(String_getHashCode, 1)                                                     \
   V(String_getLength, 1)                                                       \
   V(String_charAt, 2)                                                          \
-  V(String_codeUnitAt, 2)                                                      \
   V(String_concat, 2)                                                          \
   V(String_fromEnvironment, 3)                                                 \
   V(String_toLowerCase, 1)                                                     \

runtime/vm/compiler/backend/flow_graph_compiler.cc

@@ -3156,8 +3156,25 @@ void NullErrorSlowPath::EmitSharedStubCall(FlowGraphCompiler* compiler,
 void RangeErrorSlowPath::PushArgumentsForRuntimeCall(
     FlowGraphCompiler* compiler) {
   LocationSummary* locs = instruction()->locs();
-  __ PushRegisterPair(locs->in(CheckBoundBaseInstr::kIndexPos).reg(),
-                      locs->in(CheckBoundBaseInstr::kLengthPos).reg());
+  if (GenericCheckBoundInstr::UseUnboxedRepresentation()) {
+    // Can't pass unboxed int64 value directly to runtime call, as all
+    // arguments are expected to be tagged (boxed).
+    // The unboxed int64 argument is passed through a dedicated slot in Thread.
+    // TODO(dartbug.com/33549): Clean this up when unboxed values
+    // could be passed as arguments.
+    __ StoreToOffset(
+        locs->in(CheckBoundBaseInstr::kLengthPos).reg(),
+        compiler::Address(
+            THR, compiler::target::Thread::unboxed_runtime_arg_offset()));
+    __ StoreToOffset(
+        locs->in(CheckBoundBaseInstr::kIndexPos).reg(),
+        compiler::Address(
+            THR, compiler::target::Thread::unboxed_runtime_arg_offset() +
+                     kInt64Size));
+  } else {
+    __ PushRegisterPair(locs->in(CheckBoundBaseInstr::kIndexPos).reg(),
+                        locs->in(CheckBoundBaseInstr::kLengthPos).reg());
+  }
 }
 
 void RangeErrorSlowPath::EmitSharedStubCall(FlowGraphCompiler* compiler,

runtime/vm/compiler/backend/flow_graph_compiler.h

@@ -273,10 +273,17 @@ class NullErrorSlowPath : public ThrowErrorSlowPathCode {
 class RangeErrorSlowPath : public ThrowErrorSlowPathCode {
  public:
   explicit RangeErrorSlowPath(GenericCheckBoundInstr* instruction)
-      : ThrowErrorSlowPathCode(instruction, kRangeErrorRuntimeEntry) {}
+      : ThrowErrorSlowPathCode(
+            instruction,
+            GenericCheckBoundInstr::UseUnboxedRepresentation()
+                ? kRangeErrorUnboxedInt64RuntimeEntry
+                : kRangeErrorRuntimeEntry) {}
   virtual const char* name() { return "check bound"; }
 
   virtual intptr_t GetNumberOfArgumentsForRuntimeCall() {
+    if (GenericCheckBoundInstr::UseUnboxedRepresentation()) {
+      return 0;  // Unboxed arguments are passed through Thread.
+    }
     return 2;  // length and index
   }
 

runtime/vm/compiler/backend/inliner.cc

@@ -3166,14 +3166,14 @@ static bool InlineStringBaseCharAt(FlowGraph* flow_graph,
   return true;
 }
 
-static bool InlineStringCodeUnitAt(FlowGraph* flow_graph,
-                                   Instruction* call,
-                                   Definition* receiver,
-                                   intptr_t cid,
-                                   GraphEntryInstr* graph_entry,
-                                   FunctionEntryInstr** entry,
-                                   Instruction** last,
-                                   Definition** result) {
+static bool InlineStringBaseCodeUnitAt(FlowGraph* flow_graph,
+                                       Instruction* call,
+                                       Definition* receiver,
+                                       intptr_t cid,
+                                       GraphEntryInstr* graph_entry,
+                                       FunctionEntryInstr** entry,
+                                       Instruction** last,
+                                       Definition** result) {
   if (cid == kDynamicCid) {
     ASSERT(call->IsStaticCall());
     return false;
@@ -3220,8 +3220,11 @@ bool FlowGraphInliner::TryReplaceInstanceCallWithInline(
     ASSERT((last != nullptr && result != nullptr) ||
            (target.recognized_kind() == MethodRecognizer::kObjectConstructor));
     // Determine if inlining instance methods needs a check.
+    // StringBase.codeUnitAt is monomorphic but its implementation is selected
+    // based on the receiver cid.
     FlowGraph::ToCheck check = FlowGraph::ToCheck::kNoCheck;
-    if (target.is_polymorphic_target()) {
+    if (target.is_polymorphic_target() ||
+        (target.recognized_kind() == MethodRecognizer::kStringBaseCodeUnitAt)) {
       check = FlowGraph::ToCheck::kCheckCid;
     } else {
       check = flow_graph->CheckForInstanceCall(call, target.kind());
@@ -4274,10 +4277,10 @@ bool FlowGraphInliner::TryInlineRecognizedMethod(
       return InlineSetIndexed(flow_graph, kind, target, call, receiver, source,
                               exactness, graph_entry, entry, last, result);
     }
-    case MethodRecognizer::kOneByteStringCodeUnitAt:
-    case MethodRecognizer::kTwoByteStringCodeUnitAt:
-      return InlineStringCodeUnitAt(flow_graph, call, receiver, receiver_cid,
-                                    graph_entry, entry, last, result);
+    case MethodRecognizer::kStringBaseCodeUnitAt:
+      return InlineStringBaseCodeUnitAt(flow_graph, call, receiver,
+                                        receiver_cid, graph_entry, entry, last,
+                                        result);
     case MethodRecognizer::kStringBaseCharAt:
       return InlineStringBaseCharAt(flow_graph, call, receiver, receiver_cid,
                                     graph_entry, entry, last, result);

runtime/vm/compiler/frontend/base_flow_graph_builder.cc

@@ -397,6 +397,14 @@ Fragment BaseFlowGraphBuilder::LoadIndexed(classid_t class_id,
   return Fragment(instr);
 }
 
+Fragment BaseFlowGraphBuilder::GenericCheckBound() {
+  Value* index = Pop();
+  Value* length = Pop();
+  auto* instr = new (Z) GenericCheckBoundInstr(length, index, GetNextDeoptId());
+  Push(instr);
+  return Fragment(instr);
+}
+
 Fragment BaseFlowGraphBuilder::LoadUntagged(intptr_t offset) {
   Value* object = Pop();
   auto load = new (Z) LoadUntaggedInstr(object, offset);

runtime/vm/compiler/frontend/base_flow_graph_builder.h

@@ -188,6 +188,7 @@ class BaseFlowGraphBuilder {
                        intptr_t index_scale = compiler::target::kWordSize,
                        bool index_unboxed = false,
                        AlignmentType alignment = kAlignedAccess);
+  Fragment GenericCheckBound();
 
   Fragment LoadUntagged(intptr_t offset);
   Fragment CalculateElementAddress(intptr_t index_scale);

runtime/vm/compiler/frontend/kernel_to_il.cc

@@ -1058,6 +1058,7 @@ bool FlowGraphBuilder::IsRecognizedMethodForFlowGraph(
     case MethodRecognizer::kFinalizerEntry_allocate:
     case MethodRecognizer::kFinalizerEntry_getExternalSize:
     case MethodRecognizer::kObjectEquals:
+    case MethodRecognizer::kStringBaseCodeUnitAt:
     case MethodRecognizer::kStringBaseLength:
     case MethodRecognizer::kStringBaseIsEmpty:
     case MethodRecognizer::kClassIDgetID:
@@ -1113,6 +1114,17 @@ bool FlowGraphBuilder::IsRecognizedMethodForFlowGraph(
   }
 }
 
+bool FlowGraphBuilder::IsExpressionTempVarUsedInRecognizedMethodFlowGraph(
+    const Function& function) {
+  ASSERT(IsRecognizedMethodForFlowGraph(function));
+  switch (function.recognized_kind()) {
+    case MethodRecognizer::kStringBaseCodeUnitAt:
+      return true;
+    default:
+      return false;
+  }
+}
+
 FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
     const Function& function) {
   ASSERT(IsRecognizedMethodForFlowGraph(function));
@@ -1292,6 +1304,51 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
       body += LoadLocal(parsed_function_->RawParameterVariable(1));
       body += StrictCompare(Token::kEQ_STRICT);
       break;
+    case MethodRecognizer::kStringBaseCodeUnitAt: {
+      ASSERT_EQUAL(function.NumParameters(), 2);
+      body += LoadLocal(parsed_function_->RawParameterVariable(0));
+      body += LoadNativeField(Slot::String_length());
+      body += LoadLocal(parsed_function_->RawParameterVariable(1));
+      body += GenericCheckBound();
+      LocalVariable* safe_index = MakeTemporary();
+
+      JoinEntryInstr* done = BuildJoinEntry();
+      LocalVariable* result = parsed_function_->expression_temp_var();
+      TargetEntryInstr* one_byte_string;
+      TargetEntryInstr* two_byte_string;
+      body += LoadLocal(parsed_function_->RawParameterVariable(0));
+      body += LoadClassId();
+      body += IntConstant(kOneByteStringCid);
+      body += BranchIfEqual(&one_byte_string, &two_byte_string);
+
+      body.current = one_byte_string;
+      body += LoadLocal(parsed_function_->RawParameterVariable(0));
+      body += LoadLocal(safe_index);
+      body += LoadIndexed(
+          kOneByteStringCid,
+          /*index_scale=*/
+          compiler::target::Instance::ElementSizeFor(kOneByteStringCid),
+          /*index_unboxed=*/GenericCheckBoundInstr::UseUnboxedRepresentation());
+      body += StoreLocal(TokenPosition::kNoSource, result);
+      body += Drop();
+      body += Goto(done);
+
+      body.current = two_byte_string;
+      body += LoadLocal(parsed_function_->RawParameterVariable(0));
+      body += LoadLocal(safe_index);
+      body += LoadIndexed(
+          kTwoByteStringCid,
+          /*index_scale=*/
+          compiler::target::Instance::ElementSizeFor(kTwoByteStringCid),
+          /*index_unboxed=*/GenericCheckBoundInstr::UseUnboxedRepresentation());
+      body += StoreLocal(TokenPosition::kNoSource, result);
+      body += Drop();
+      body += Goto(done);
+
+      body.current = done;
+      body += DropTemporary(&safe_index);
+      body += LoadLocal(result);
+    } break;
     case MethodRecognizer::kStringBaseLength:
     case MethodRecognizer::kStringBaseIsEmpty:
       ASSERT_EQUAL(function.NumParameters(), 1);

runtime/vm/compiler/frontend/kernel_to_il.h

@@ -60,6 +60,11 @@ class FlowGraphBuilder : public BaseFlowGraphBuilder {
   // graph building and its body is expressed in a custom-built IL.
   static bool IsRecognizedMethodForFlowGraph(const Function& function);
 
+  // Returns true if custom flow graph for given [function]
+  // needs an expression_temp_var().
+  static bool IsExpressionTempVarUsedInRecognizedMethodFlowGraph(
+      const Function& function);
+
  private:
   BlockEntryInstr* BuildPrologue(BlockEntryInstr* normal_entry,
                                  PrologueInfo* prologue_info);

runtime/vm/compiler/frontend/scope_builder.cc

@@ -5,6 +5,7 @@
 #include "vm/compiler/frontend/scope_builder.h"
 
 #include "vm/compiler/backend/il.h"  // For CompileType.
+#include "vm/compiler/frontend/kernel_to_il.h"
 #include "vm/compiler/frontend/kernel_translation_helper.h"
 
 namespace dart {
@@ -167,6 +168,11 @@ ScopeBuildingResult* ScopeBuilder::BuildScopes() {
           --depth_.catch_;
         }
       }
+      if (FlowGraphBuilder::IsRecognizedMethodForFlowGraph(function) &&
+          FlowGraphBuilder::IsExpressionTempVarUsedInRecognizedMethodFlowGraph(
+              function)) {
+        needs_expr_temp_ = true;
+      }
       intptr_t pos = 0;
       if (function.IsClosureFunction()) {
         LocalVariable* closure_parameter = MakeVariable(

runtime/vm/compiler/graph_intrinsifier.cc

@@ -431,49 +431,6 @@ DEFINE_SIMD_ARRAY_GETTER_SETTER_INTRINSICS(Float64x2Array)
 #undef DEFINE_SIMD_ARRAY_GETTER_INTRINSIC
 #undef DEFINE_SIMD_ARRAY_SETTER_INTRINSIC
 
-static bool BuildCodeUnitAt(FlowGraph* flow_graph, intptr_t cid) {
-  GraphEntryInstr* graph_entry = flow_graph->graph_entry();
-  auto normal_entry = graph_entry->normal_entry();
-  BlockBuilder builder(flow_graph, normal_entry, /*with_frame=*/false);
-
-  Definition* str = builder.AddParameter(0);
-  Definition* index = builder.AddParameter(1);
-
-  VerifyParameterIsBoxed(&builder, 0);
-
-  index = CreateBoxedParameterIfNeeded(&builder, index, kUnboxedInt64, 1);
-  index =
-      PrepareIndexedOp(flow_graph, &builder, str, index, Slot::String_length());
-
-  Definition* load = builder.AddDefinition(new LoadIndexedInstr(
-      new Value(str), new Value(index), /*index_unboxed=*/false,
-      target::Instance::ElementSizeFor(cid), cid, kAlignedAccess,
-      DeoptId::kNone, builder.Source()));
-
-  // We don't perform [RangeAnalysis] for graph intrinsics. To inform the
-  // following boxing instruction about a more precise range we attach it here
-  // manually.
-  // http://dartbug.com/36632
-  auto const rep = RepresentationUtils::RepresentationOfArrayElement(cid);
-  load->set_range(Range::Full(rep));
-  Definition* result = CreateBoxedResultIfNeeded(&builder, load, rep);
-
-  if (result->IsBoxInteger()) {
-    result->AsBoxInteger()->ClearEnv();
-  }
-
-  builder.AddReturn(new Value(result));
-  return true;
-}
-
-bool GraphIntrinsifier::Build_OneByteStringCodeUnitAt(FlowGraph* flow_graph) {
-  return BuildCodeUnitAt(flow_graph, kOneByteStringCid);
-}
-
-bool GraphIntrinsifier::Build_TwoByteStringCodeUnitAt(FlowGraph* flow_graph) {
-  return BuildCodeUnitAt(flow_graph, kTwoByteStringCid);
-}
-
 static bool BuildSimdOp(FlowGraph* flow_graph, intptr_t cid, Token::Kind kind) {
   if (!FlowGraphCompiler::SupportsUnboxedSimd128()) return false;
 

runtime/vm/compiler/recognized_methods_list.h

@@ -124,6 +124,7 @@ namespace dart {
   V(::, copyRangeFromUint8ListToOneByteString,                                 \
     CopyRangeFromUint8ListToOneByteString, 0xcc42d0a2)                         \
   V(_StringBase, _interpolate, StringBaseInterpolate, 0x8af456e6)              \
+  V(_StringBase, codeUnitAt, StringBaseCodeUnitAt, 0x17ea80f1)                 \
   V(_SuspendState, get:_functionData, SuspendState_getFunctionData,            \
     0x7281768e)                                                                \
   V(_SuspendState, set:_functionData, SuspendState_setFunctionData,            \
@@ -452,8 +453,6 @@ namespace dart {
   V(_GrowableList, [], GrowableArrayGetIndexed, 0x78e668b1)                    \
   V(_GrowableList, _setIndexed, GrowableArraySetIndexedUnchecked, 0x513c774f)  \
   V(_StringBase, get:length, StringBaseLength, 0x5842648b)                     \
-  V(_OneByteString, codeUnitAt, OneByteStringCodeUnitAt, 0x17ea7d30)           \
-  V(_TwoByteString, codeUnitAt, TwoByteStringCodeUnitAt, 0x17ea7d30)           \
   V(_Smi, ~, Smi_bitNegate, 0x82466cfc)                                        \
   V(_IntegerImplementation, +, Integer_add, 0x6f06d26c)                        \
   V(_IntegerImplementation, -, Integer_sub, 0x630fe15d)                        \