Make store-to-load forwarding work with FFI Pointers

[mkustermann]

Once we allow inlining force optimized functions (such as Pointer.fromAddress()) we seem to generate for this dart code

import 'dart:ffi';

main() {
  for (int i = 0; i < 10000; ++i) {
    foo();
  }
}

@pragma('vm:never-inline')
int foo() {
  return Pointer<Void>.fromAddress(0).address;
}

this flow graph

out/ReleaseX64/dart --print-flow-graph-optimized --print-flow-graph-filter=foo ffi.dart
*** BEGIN CFG
After AllocateRegisters
==== file:///.../sdk/ffi.dart_::_foo (RegularFunction)
  0: B0[graph]:0 {
      v0 <- Constant(#null) T{Null?}
      v1 <- Constant(#<optimized out>) T{Sentinel~}
      v2 <- Constant(#TypeArguments: (H116f861f) [Type: Void]) T{TypeArguments}
      v3 <- Constant(#0) [0, 0] T{_Smi}
      v32 <- Constant(#TypeArguments: (H39d2e3b4) [Type: Never]) T{TypeArguments}
      v35 <- UnboxedConstant(#0 int64) [0, 0] T{_Smi}
}
  2: B1[function entry]:2
  3:     ParallelMove rdx <- C
  4:     v28 <- AllocateObject:12(cls=Pointer, v32 T{TypeArguments}) T{Pointer}
  5:     ParallelMove rax <- rax, rcx <- C
  6:     StoreField(v28 . PointerBase.data = v35 T{_Smi} <int64>, NoStoreBarrier)
  8:     v18 <- LoadUntagged(v28 T{Pointer}, 8) T{*?}
 10:     v19 <- IntConverter(untagged->int64[tr], v18) T{int}
 12:     v20 <- BoxInt64(v19) T{int}
 14:     Return:18(v20 T{int})
*** END CFG

Instead it should just return a constant.

/cc @alexmarkov

[mkustermann]

It does seem strange that we use StoreField to store but use LoadUntagged to load.

[mraleph]

I think @ds84182 and @dcharkes were looking at this: #52510

I am not sure why this stalled.

[dcharkes]

I think @ds84182 and @dcharkes were looking at this: #52510

I am not sure why this stalled.

There are a bunch of test failures which haven't been triaged and addressed: https://dart-ci.firebaseapp.com/cl/305641/3.

[blaugold]

I noticed that a few test failures had to do with Pointer.address. Shouldn't this Box instruction in the code generated for MethodRecognizer::kFfiGetAddress use kUnboxedIntPtr as the representation, since that is the representation of Slot::PointerBase_data?

[dcharkes]

Interesting, when merging the representations, we merge two non-equal representations:

  V(Pointer, UntaggedPointer, data_field, FfiIntPtr, FINAL)                    \
  V(TypedDataBase, UntaggedTypedDataBase, data_field, IntPtr, VAR)             \

to

  V(PointerBase, UntaggedPointerBase, data, IntPtr, VAR)

in https://dart-review.googlesource.com/c/sdk/+/231160 cc @alexmarkov

I don't believe we use the TypedData data field as int value, but we do use the Pointer data field as int value when doing .address. So the fix should be:

  V(PointerBase, UntaggedPointerBase, data, FfiIntPtr, VAR)

Though, that might have some unintended side effects.

[alexmarkov]

@dcharkes What is the point of having distinct kUnboxedFfiIntPtr and kUnboxedIntPtr? Why kUnboxedFfiIntPtr is unsigned on 32-bit systems despite its name (IntPtr and not UintPtr)? Note that it could be more efficient to keep 32-bit addresses sign-extended when represented with Dart int, as they are more likely to fit into Smi range. #36370 looks related.

When changing the representation of a slot (such as Slot::PointerBase_data()) we need to make sure that all accesses to that slot (loads and stores) agree with its representation.

[dcharkes]

@dcharkes What is the point of having distinct kUnboxedFfiIntPtr and kUnboxedIntPtr? Why kUnboxedFfiIntPtr is unsigned on 32-bit systems despite its name (IntPtr and not UintPtr)?

// 'UnboxedFfiIntPtr' should be able to hold a pointer of the target word-size.
// On a 32-bit platform, it's an unsigned 32-bit int because it should be
// zero-extended to 64-bits, not sign-extended (pointers are inherently
// unsigned).

Introduced in https://dart-review.googlesource.com/c/sdk/+/94750 and https://dart-review.googlesource.com/c/sdk/+/94860.

There is already a TODO as you mention: #36370. I don't remember the motivation for #36370 (comment). @mkustermann do you? Is it that all pointers are always unsigned but we don't have a 64 bit unsigned representation? (e.g. https://dart-review.googlesource.com/c/sdk/+/195920 also defines unboxed-uword as unboxed-word.)

Note that it could be more efficient to keep 32-bit addresses sign-extended when represented with Dart int, as they are more likely to fit into Smi range.

The field is unboxed right? So it doesn't matter whether it's a Smi.

It would be a breaking change if we start showing 32 bit Pointer.addresses to users as signed.

To prevent a breaking change, we'd need to insert int-converter instructions at the loads and stores if we use kUnboxedIntPtr. This might be labour intensive, we have quite a lot of kUnboxedFfiIntPtr and kFfiIntPtr uses in the codebase.

When changing the representation of a slot (such as Slot::PointerBase_data()) we need to make sure that all accesses to that slot (loads and stores) agree with its representation.

Which might be less or more labour intensive. then covering all kUnboxedFfiIntPtr cases.

I noticed that a few test failures had to do with Pointer.address. Shouldn't this Box instruction in the code generated for MethodRecognizer::kFfiGetAddress use kUnboxedIntPtr as the representation, since that is the representation of Slot::PointerBase_data?

@blaugold Try adding IntConverter instructions in the places where you've removed ConvertUntaggedToUnboxed(kUnboxedIntPtr);. These are less places than completely removing the whole kUnboxedFfiIntPtr, and should enable you to move forward with the PR.

[blaugold]

@blaugold Try adding IntConverter instructions in the places where you've removed ConvertUntaggedToUnboxed(kUnboxedIntPtr);. These are less places than completely removing the whole kUnboxedFfiIntPtr, and should enable you to move forward with the PR.

It's not my PR, I just took a look at the test failures and the code to see what might be going on. :) Hopefully @ds84182 is still interested in working on it.

[ds84182]

I'm still interested in working on it. I thought my latest commit fixed the issue but it seems like it hasn't :c

[mkustermann]

There are a bunch of test failures which haven't been triaged and addressed: https://dart-ci.firebaseapp.com/cl/305641/3.

@dcharkes then we should diagnose and fix them :-)

I noticed that a few test failures had to do with Pointer.address. Shouldn't this Box instruction in the code generated for MethodRecognizer::kFfiGetAddress use kUnboxedIntPtr as the representation, since that is the representation of Slot::PointerBase_data?

I think @blaugold is spot on.

Here's a small reproduction for ia32 with the current state of the CL:

import 'dart:ffi';

import "package:expect/expect.dart";
import "package:ffi/ffi.dart";

void main() {
  while (true) {
    testPointerPointerArithmeticSizes();
  }
}

Pointer<Int64> int64Array = calloc(2);
void testPointerPointerArithmeticSizes() {
  Expect.equals(int64Array.address + 8, int64Array.elementAt(1).address);
}

% tools/build.py -nvh -d0 -mrelease -aia32 runtime
...
% out/ReleaseIA32/dart test.dart
Unhandled exception:
Expect.equals(expected: <-235902232>, actual: <4059065064>) fails.
...

This happens because once we have a 32-bit address and sign-extend it int32 to int64 do arithmetic and BoxInt64(), and once we truncate to u32 and then BoxUint32() it.

I think in the end it doesn't matter so much whether we represent C pointer addresses (i.e. Pointer<>.address) as sign-extended or not: Whenever we use it (e.g. to load/store memory, to pass to C code) we will only use the lower 32 bits. What matters is that we consistently go from the 32-bits in the Pointer object to the Dart int (64-bit) integer.

[blaugold]

Try adding IntConverter instructions in the places where you've removed ConvertUntaggedToUnboxed(kUnboxedIntPtr);. These are less places than completely removing the whole kUnboxedFfiIntPtr, and should enable you to move forward with the PR.

@dcharkes Did you mean something like that?

diff --git a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
index d152d7b172b..7d69a63d4a9 100644
--- a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
+++ b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
@@ -427,6 +427,18 @@ Fragment BaseFlowGraphBuilder::ConvertUnboxedToUntagged(
   return Fragment(converted);
 }
 
+Fragment BaseFlowGraphBuilder::ConvertUnboxedIntPtrToFfi() {
+  if (kUnboxedIntPtr == kUnboxedFfiIntPtr) {
+    return Fragment();
+  }
+  Value* value = Pop();
+  auto converted = new (Z) IntConverterInstr(kUnboxedIntPtr, kUnboxedFfiIntPtr,
+                                             value, DeoptId::kNone);
+  converted->mark_truncating();
+  Push(converted);
+  return Fragment(converted);
+}
+
 Fragment BaseFlowGraphBuilder::AddIntptrIntegers() {
   Value* right = Pop();
   Value* left = Pop();
diff --git a/runtime/vm/compiler/frontend/base_flow_graph_builder.h b/runtime/vm/compiler/frontend/base_flow_graph_builder.h
index e6a13747769..198b4f814e9 100644
--- a/runtime/vm/compiler/frontend/base_flow_graph_builder.h
+++ b/runtime/vm/compiler/frontend/base_flow_graph_builder.h
@@ -185,6 +185,7 @@ class BaseFlowGraphBuilder {
   Fragment LoadUntagged(intptr_t offset);
   Fragment ConvertUntaggedToUnboxed(Representation to);
   Fragment ConvertUnboxedToUntagged(Representation from);
+  Fragment ConvertUnboxedIntPtrToFfi();
   Fragment UnboxSmiToIntptr();
   Fragment FloatToDouble();
   Fragment DoubleToFloat();
diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc
index 9c31e4d6629..ec9f434862f 100644
--- a/runtime/vm/compiler/frontend/kernel_to_il.cc
+++ b/runtime/vm/compiler/frontend/kernel_to_il.cc
@@ -1381,6 +1381,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
       body += LoadLocal(arg_value_not_null);
       if (kind == MethodRecognizer::kFfiStorePointer) {
         body += LoadNativeField(Slot::PointerBase_data());
+        body += ConvertUnboxedIntPtrToFfi();
       } else {
         // Avoid any unnecessary (and potentially deoptimizing) int
         // conversions by using the representation consumed by StoreIndexed.
@@ -1418,6 +1419,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
       body += LoadLocal(parsed_function_->RawParameterVariable(0));  // Pointer.
       body += CheckNullOptimized(String::ZoneHandle(Z, function.name()));
       body += LoadNativeField(Slot::PointerBase_data());
+      body += ConvertUnboxedIntPtrToFfi();
       body += Box(kUnboxedFfiIntPtr);
     } break;
     case MethodRecognizer::kHas63BitSmis: {
@@ -4712,6 +4714,7 @@ Fragment FlowGraphBuilder::FfiConvertPrimitiveToNative(
   Fragment body;
   if (marshaller.IsPointer(arg_index)) {
     body += LoadNativeField(Slot::PointerBase_data());
+    body += ConvertUnboxedIntPtrToFfi();
   } else if (marshaller.IsHandle(arg_index)) {
     body += WrapHandle();
   } else {
@@ -4848,6 +4851,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) {
                     ->context_variables()[0]));
 
   body += LoadNativeField(Slot::PointerBase_data());
+  body += ConvertUnboxedIntPtrToFfi();
 
   if (marshaller.PassTypedData()) {
     body += LoadLocal(typed_data);

[dcharkes]

Currently, adding ConvertUnboxedIntPtrToFfi doesn't work, because our optimizer is a bit too eager and removes an intconverter from the chain in the example that Martin constructed:

    v22 <- LoadField(v3 T{Pointer} . PointerBase.data) T{int}
    v23 <- IntConverter(int32->uint32[tr], v22) T{int}
    v24 <- BoxUint32(v23) T{int}
    v74 <- UnboxInt64:34([guard-inputs], v11) T{int}
    v75 <- UnboxInt64:34([guard-inputs], v24 T{int}) T{int}
    v12 <- BinaryInt64Op:34(+ [tr], v75 T{int}, v74 T{_Smi}) T{int}

gets turned into

    v22 <- LoadField(v3 T{Pointer} . PointerBase.data) T{int}
    v23 <- IntConverter(int32->uint32[tr], v22) T{int}
    v82 <- IntConverter(int32->int64, v22) T{int}
    v12 <- BinaryInt64Op:34(+ [tr], v82 T{int}, v79 T{_Smi}) T{int}

I'll see what I can do to push this PR over the line.