[vm/ffi] Unbox Pointer data field

This regresses Pointer load and store loops because LoadUntagged is never hoisted out of loops. But with support for passing TypedData, in a follow up CL, we cannot assume that only Pointers are passed in the future.

It also changes the FfiCallInstr context to hold the Pointer, rather than the address, as call sites can be unoptimized and we cannot handle an untagged address there.

Issue: dart-lang/sdk#40767
Issue: dart-lang/sdk#36730
Change-Id: Icc716d79eb9eb2b5aac4f03dbf6c622a6825ffdc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/137793
Reviewed-by: Martin Kustermann <[email protected]>

Author: dcharkes

runtime/vm/compiler/backend/il.cc

@@ -2620,7 +2620,7 @@ bool LoadFieldInstr::IsImmutableLengthLoad() const {
     case Slot::Kind::kClosure_hash:
     case Slot::Kind::kCapturedVariable:
     case Slot::Kind::kDartField:
-    case Slot::Kind::kPointer_c_memory_address:
+    case Slot::Kind::kPointer_data:
     case Slot::Kind::kType_arguments:
     case Slot::Kind::kTypeArgumentsIndex:
       return false;
@@ -3025,6 +3025,9 @@ Definition* BoxInt64Instr::Canonicalize(FlowGraph* flow_graph) {
   // Find a more precise box instruction.
   if (auto conv = value()->definition()->AsIntConverter()) {
     Definition* replacement;
+    if (conv->from() == kUntagged) {
+      return this;
+    }
     switch (conv->from()) {
       case kUnboxedInt32:
         replacement = new BoxInt32Instr(conv->value()->CopyWithType());
@@ -3170,6 +3173,14 @@ Definition* IntConverterInstr::Canonicalize(FlowGraph* flow_graph) {
       return this;
     }
 
+#if defined(TARGET_ARCH_IS_32_BIT)
+    // Do not erase extending conversions from 32-bit untagged to 64-bit values
+    // because untagged does not specify whether it is signed or not.
+    if ((box_defn->from() == kUntagged) && to() == kUnboxedInt64) {
+      return this;
+    }
+#endif
+
     if (box_defn->from() == to()) {
       return box_defn->value()->definition();
     }

runtime/vm/compiler/backend/range_analysis.cc

@@ -2703,7 +2703,7 @@ void LoadFieldInstr::InferRange(RangeAnalysis* analysis, Range* range) {
     case Slot::Kind::kClosure_function:
     case Slot::Kind::kClosure_function_type_arguments:
     case Slot::Kind::kClosure_instantiator_type_arguments:
-    case Slot::Kind::kPointer_c_memory_address:
+    case Slot::Kind::kPointer_data:
     case Slot::Kind::kTypedDataBase_data_field:
     case Slot::Kind::kTypedDataView_data:
     case Slot::Kind::kType_arguments:

runtime/vm/compiler/backend/slot.h

@@ -72,7 +72,7 @@ class ParsedFunction;
   V(ArgumentsDescriptor, positional_count, Smi, FINAL)                         \
   V(ArgumentsDescriptor, count, Smi, FINAL)                                    \
   V(ArgumentsDescriptor, size, Smi, FINAL)                                     \
-  V(Pointer, c_memory_address, Dynamic, FINAL)                                 \
+  V(Pointer, data, Dynamic, FINAL)                                             \
   V(Type, arguments, TypeArguments, FINAL)
 
 // Slot is an abstraction that describes an readable (and possibly writeable)

runtime/vm/compiler/frontend/base_flow_graph_builder.cc

@@ -911,6 +911,12 @@ Fragment BaseFlowGraphBuilder::AllocateObject(TokenPosition position,
   return Fragment(allocate);
 }
 
+Fragment BaseFlowGraphBuilder::Box(Representation from) {
+  BoxInstr* box = BoxInstr::Create(from, Pop());
+  Push(box);
+  return Fragment(box);
+}
+
 Fragment BaseFlowGraphBuilder::BuildFfiAsFunctionInternalCall(
     const TypeArguments& signatures) {
   ASSERT(signatures.IsInstantiated());
@@ -926,16 +932,17 @@ Fragment BaseFlowGraphBuilder::BuildFfiAsFunctionInternalCall(
           Function::Handle(Z, Type::Cast(native_type).signature())));
 
   Fragment code;
-  code += LoadNativeField(Slot::Pointer_c_memory_address());
-  LocalVariable* address = MakeTemporary();
+  // Store the pointer in the context, we cannot load the untagged address
+  // here as these can be unoptimized call sites.
+  LocalVariable* pointer = MakeTemporary();
 
   auto& context_slots = CompilerState::Current().GetDummyContextSlots(
       /*context_id=*/0, /*num_variables=*/1);
   code += AllocateContext(context_slots);
   LocalVariable* context = MakeTemporary();
 
   code += LoadLocal(context);
-  code += LoadLocal(address);
+  code += LoadLocal(pointer);
   code += StoreInstanceField(TokenPosition::kNoSource, *context_slots[0]);
 
   code += AllocateClosure(TokenPosition::kNoSource, target);

runtime/vm/compiler/frontend/base_flow_graph_builder.h

@@ -205,6 +205,9 @@ class BaseFlowGraphBuilder {
                                  intptr_t index_scale,
                                  bool index_unboxed);
 
+  // Sign-extends kUnboxedInt32 and zero-extends kUnboxedUint32.
+  Fragment Box(Representation from);
+
   void Push(Definition* definition);
   Definition* Peek(intptr_t depth = 0);
   Value* Pop();

runtime/vm/compiler/frontend/kernel_to_il.cc

@@ -1215,15 +1215,17 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
       LocalVariable* arg_pointer = parsed_function_->RawParameterVariable(0);
       LocalVariable* arg_offset = parsed_function_->RawParameterVariable(1);
 
-      body += LoadLocal(arg_pointer);
+      body += LoadLocal(arg_offset);
       body += CheckNullOptimized(TokenPosition::kNoSource,
                                  String::ZoneHandle(Z, function.name()));
-      body += LoadNativeField(Slot::Pointer_c_memory_address());
-      body += UnboxTruncate(kUnboxedFfiIntPtr);
-      body += ConvertUnboxedToUntagged(kUnboxedFfiIntPtr);
-      body += LoadLocal(arg_offset);
+      LocalVariable* arg_offset_not_null = MakeTemporary();
+
+      body += LoadLocal(arg_pointer);
       body += CheckNullOptimized(TokenPosition::kNoSource,
                                  String::ZoneHandle(Z, function.name()));
+      // No GC from here til LoadIndexed.
+      body += LoadUntagged(compiler::target::Pointer::data_offset());
+      body += LoadLocal(arg_offset_not_null);
       body += UnboxTruncate(kUnboxedFfiIntPtr);
       body += LoadIndexedTypedData(typed_data_cid, /*index_scale=*/1,
                                    /*index_unboxed=*/true);
@@ -1263,11 +1265,13 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
           LocalVariable* pointer = MakeTemporary();
           body += LoadLocal(pointer);
           body += LoadLocal(address);
-          body += StoreInstanceField(TokenPosition::kNoSource,
-                                     Slot::Pointer_c_memory_address());
+          body += UnboxTruncate(kUnboxedFfiIntPtr);
+          body += ConvertUnboxedToUntagged(kUnboxedFfiIntPtr);
+          body += StoreUntagged(compiler::target::Pointer::data_offset());
           body += DropTempsPreserveTop(1);  // Drop [address] keep [pointer].
         }
       }
+      body += DropTempsPreserveTop(1);  // Drop [arg_offset].
     } break;
     case MethodRecognizer::kFfiStoreInt8:
     case MethodRecognizer::kFfiStoreInt16:
@@ -1327,21 +1331,27 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
       }
 
       ASSERT(function.NumParameters() == 3);
-      body += LoadLocal(arg_pointer);  // Pointer.
+      body += LoadLocal(arg_offset);
       body += CheckNullOptimized(TokenPosition::kNoSource,
                                  String::ZoneHandle(Z, function.name()));
-      body += LoadNativeField(Slot::Pointer_c_memory_address());
-      body += UnboxTruncate(kUnboxedFfiIntPtr);
-      body += ConvertUnboxedToUntagged(kUnboxedFfiIntPtr);
-      body += LoadLocal(arg_offset);  // Offset.
+      LocalVariable* arg_offset_not_null = MakeTemporary();
+      body += LoadLocal(arg_value);
       body += CheckNullOptimized(TokenPosition::kNoSource,
                                  String::ZoneHandle(Z, function.name()));
-      body += UnboxTruncate(kUnboxedFfiIntPtr);
-      body += LoadLocal(arg_value);  // Value.
+      LocalVariable* arg_value_not_null = MakeTemporary();
+
+      body += LoadLocal(arg_pointer);  // Pointer.
       body += CheckNullOptimized(TokenPosition::kNoSource,
                                  String::ZoneHandle(Z, function.name()));
+      // No GC from here til StoreIndexed.
+      body += LoadUntagged(compiler::target::Pointer::data_offset());
+      body += LoadLocal(arg_offset_not_null);
+      body += UnboxTruncate(kUnboxedFfiIntPtr);
+      body += LoadLocal(arg_value_not_null);
       if (kind == MethodRecognizer::kFfiStorePointer) {
-        body += LoadNativeField(Slot::Pointer_c_memory_address());
+        // This can only be Pointer, so it is always safe to LoadUntagged.
+        body += LoadUntagged(compiler::target::Pointer::data_offset());
+        body += ConvertUntaggedToUnboxed(kUnboxedFfiIntPtr);
       } else if (kind == MethodRecognizer::kFfiStoreFloat ||
                  kind == MethodRecognizer::kFfiStoreDouble) {
         body += UnboxTruncate(kUnboxedDouble);
@@ -1353,6 +1363,8 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
       }
       body += StoreIndexedTypedData(typed_data_cid, /*index_scale=*/1,
                                     /*index_unboxed=*/true);
+      body += Drop();  // Drop [arg_value].
+      body += Drop();  // Drop [arg_offset].
       body += NullConstant();
     } break;
     case MethodRecognizer::kFfiFromAddress: {
@@ -1369,21 +1381,19 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
       body += LoadLocal(parsed_function_->RawParameterVariable(0));  // Address.
       body += CheckNullOptimized(TokenPosition::kNoSource,
                                  String::ZoneHandle(Z, function.name()));
-#if defined(TARGET_ARCH_IS_32_BIT)
-      // Truncate to 32 bits on 32 bit architecture.
       body += UnboxTruncate(kUnboxedFfiIntPtr);
-      body += Box(kUnboxedFfiIntPtr);
-#endif  // defined(TARGET_ARCH_IS_32_BIT)
-      body += StoreInstanceField(TokenPosition::kNoSource,
-                                 Slot::Pointer_c_memory_address(),
-                                 StoreInstanceFieldInstr::Kind::kInitializing);
+      body += ConvertUnboxedToUntagged(kUnboxedFfiIntPtr);
+      body += StoreUntagged(compiler::target::Pointer::data_offset());
     } break;
     case MethodRecognizer::kFfiGetAddress: {
       ASSERT(function.NumParameters() == 1);
       body += LoadLocal(parsed_function_->RawParameterVariable(0));  // Pointer.
       body += CheckNullOptimized(TokenPosition::kNoSource,
                                  String::ZoneHandle(Z, function.name()));
-      body += LoadNativeField(Slot::Pointer_c_memory_address());
+      // This can only be Pointer, so it is always safe to LoadUntagged.
+      body += LoadUntagged(compiler::target::Pointer::data_offset());
+      body += ConvertUntaggedToUnboxed(kUnboxedFfiIntPtr);
+      body += Box(kUnboxedFfiIntPtr);
     } break;
     default: {
       UNREACHABLE();
@@ -2752,12 +2762,6 @@ Fragment FlowGraphBuilder::UnboxTruncate(Representation to) {
   return Fragment(unbox);
 }
 
-Fragment FlowGraphBuilder::Box(Representation from) {
-  BoxInstr* box = BoxInstr::Create(from, Pop());
-  Push(box);
-  return Fragment(box);
-}
-
 Fragment FlowGraphBuilder::NativeReturn(
     const compiler::ffi::CallbackMarshaller& marshaller) {
   auto* instr = new (Z) NativeReturnInstr(TokenPosition::kNoSource, Pop(),
@@ -2787,9 +2791,9 @@ Fragment FlowGraphBuilder::FfiPointerFromAddress(const Type& result_type) {
   LocalVariable* pointer = MakeTemporary();
   code += LoadLocal(pointer);
   code += LoadLocal(address);
-  code += StoreInstanceField(TokenPosition::kNoSource,
-                             Slot::Pointer_c_memory_address(),
-                             StoreInstanceFieldInstr::Kind::kInitializing);
+  code += UnboxTruncate(kUnboxedFfiIntPtr);
+  code += ConvertUnboxedToUntagged(kUnboxedFfiIntPtr);
+  code += StoreUntagged(compiler::target::Pointer::data_offset());
   code += StoreLocal(TokenPosition::kNoSource, result);
   code += Drop();  // StoreLocal^
   code += Drop();  // address
@@ -2837,11 +2841,13 @@ Fragment FlowGraphBuilder::FfiConvertArgumentToNative(
                              String::ZoneHandle(Z, marshaller.function_name()));
 
   if (marshaller.IsPointer(arg_index)) {
-    body += LoadNativeField(Slot::Pointer_c_memory_address());
+    // This can only be Pointer, so it is always safe to LoadUntagged.
+    body += LoadUntagged(compiler::target::Pointer::data_offset());
+    body += ConvertUntaggedToUnboxed(kUnboxedFfiIntPtr);
+  } else {
+    body += UnboxTruncate(marshaller.RepInDart(arg_index));
   }
 
-  body += UnboxTruncate(marshaller.RepInDart(arg_index));
-
   if (marshaller.RequiresBitCast(arg_index)) {
     body += BitCast(marshaller.RepInDart(arg_index),
                     marshaller.RepInFfiCall(arg_index));
@@ -2885,15 +2891,18 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) {
     body += FfiConvertArgumentToNative(marshaller, i);
   }
 
-  // Push the function pointer, which is stored (boxed) in the first slot of the
-  // context.
+  // Push the function pointer, which is stored (as Pointer object) in the
+  // first slot of the context.
   body += LoadLocal(parsed_function_->ParameterVariable(0));
   body += LoadNativeField(Slot::Closure_context());
   body += LoadNativeField(Slot::GetContextVariableSlotFor(
       thread_, *MakeImplicitClosureScope(
                     Z, Class::Handle(I->object_store()->ffi_pointer_class()))
                     ->context_variables()[0]));
-  body += UnboxTruncate(kUnboxedFfiIntPtr);
+
+  // This can only be Pointer, so it is always safe to LoadUntagged.
+  body += LoadUntagged(compiler::target::Pointer::data_offset());
+  body += ConvertUntaggedToUnboxed(kUnboxedFfiIntPtr);
   body += FfiCall(marshaller);
 
   body += FfiConvertArgumentToDart(marshaller, compiler::ffi::kResultIndex);

runtime/vm/compiler/frontend/kernel_to_il.h

@@ -190,9 +190,6 @@ class FlowGraphBuilder : public BaseFlowGraphBuilder {
   // target representation.
   Fragment UnboxTruncate(Representation to);
 
-  // Sign-extends kUnboxedInt32 and zero-extends kUnboxedUint32.
-  Fragment Box(Representation from);
-
   // Creates an ffi.Pointer holding a given address (TOS).
   Fragment FfiPointerFromAddress(const Type& result_type);
 

runtime/vm/compiler/runtime_api.h

@@ -580,7 +580,7 @@ class ArgumentsDescriptor : public AllStatic {
 
 class Pointer : public AllStatic {
  public:
-  static word c_memory_address_offset();
+  static word data_offset();
   static word type_arguments_offset();
   static word InstanceSize();
   static word NextFieldOffset();