Skip to content

Commit

Permalink
[vm/ffi] Unbox Pointer data field
Browse files Browse the repository at this point in the history 
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: #40767
Issue: #36730
Change-Id: Icc716d79eb9eb2b5aac4f03dbf6c622a6825ffdc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/137793
Reviewed-by: Martin Kustermann <kustermann@google.com>
  • Loading branch information
@dcharkes
dcharkes authored and commit-bot@chromium.org committed Mar 5, 2020
1 parent 46bc5cd commit b406802
Show file tree
Hide file tree
Showing 13 changed files with 97 additions and 87 deletions.
13 changes: 12 additions & 1 deletion runtime/vm/compiler/backend/il.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion runtime/vm/compiler/backend/range_analysis.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion runtime/vm/compiler/backend/slot.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
13 changes: 10 additions & 3 deletions runtime/vm/compiler/frontend/base_flow_graph_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions runtime/vm/compiler/frontend/base_flow_graph_builder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
85 changes: 47 additions & 38 deletions runtime/vm/compiler/frontend/kernel_to_il.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand All @@ -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: {
Expand All @@ -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();
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 0 additions & 3 deletions runtime/vm/compiler/frontend/kernel_to_il.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion runtime/vm/compiler/runtime_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading

0 comments on commit b406802

Please sign in to comment.