From abf1d96d17ef84a708ece583be05039f1b0112b3 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Fri, 13 Sep 2024 05:55:57 +0000 Subject: [PATCH 1/2] [vm/x64] Improve byte indexed stores On X64 you can use `movb` instruction with any register, so there is no need to require a fixed register. This improves the quality of register allocation decisions around such stores. Issue https://github.com/dart-lang/sdk/issues/56705 TEST=ci Change-Id: Id4d977853fec4a7900cb33c3062257735505ab8b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/384703 Reviewed-by: Daco Harkes Commit-Queue: Daco Harkes Auto-Submit: Slava Egorov --- runtime/vm/compiler/backend/il_x64.cc | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/runtime/vm/compiler/backend/il_x64.cc b/runtime/vm/compiler/backend/il_x64.cc index ffb40b8ebd38..d0ee56b6506b 100644 --- a/runtime/vm/compiler/backend/il_x64.cc +++ b/runtime/vm/compiler/backend/il_x64.cc @@ -2057,9 +2057,11 @@ LocationSummary* StoreIndexedInstr::MakeLocationSummary(Zone* zone, RepresentationUtils::RepresentationOfArrayElement(class_id()); if (RepresentationUtils::IsUnboxedInteger(rep)) { if (rep == kUnboxedUint8 || rep == kUnboxedInt8) { - // TODO(fschneider): Add location constraint for byte registers (RAX, - // RBX, RCX, RDX) instead of using a fixed register. - locs->set_in(2, LocationFixedRegisterOrSmiConstant(value(), RAX)); + if (IsClampedTypedDataBaseClassId(class_id())) { + locs->set_in(2, LocationWritableRegisterOrSmiConstant(value())); + } else { + locs->set_in(2, LocationRegisterOrSmiConstant(value())); + } } else { locs->set_in(2, Location::RequiresRegister()); } @@ -2128,18 +2130,18 @@ void StoreIndexedInstr::EmitNativeCode(FlowGraphCompiler* compiler) { } __ movb(element_address, compiler::Immediate(static_cast(value))); } else { - const Register storedValueReg = locs()->in(2).reg(); + const Register value = locs()->in(2).reg(); compiler::Label store_value, store_0xff; - __ CompareImmediate(storedValueReg, compiler::Immediate(0xFF)); + __ CompareImmediate(value, compiler::Immediate(0xFF)); __ j(BELOW_EQUAL, &store_value, compiler::Assembler::kNearJump); // Clamp to 0x0 or 0xFF respectively. __ j(GREATER, &store_0xff); - __ xorq(storedValueReg, storedValueReg); + __ xorq(value, value); __ jmp(&store_value, compiler::Assembler::kNearJump); __ Bind(&store_0xff); - __ LoadImmediate(storedValueReg, compiler::Immediate(0xFF)); + __ LoadImmediate(value, compiler::Immediate(0xFF)); __ Bind(&store_value); - __ movb(element_address, ByteRegisterOf(storedValueReg)); + __ movb(element_address, ByteRegisterOf(value)); } } else if (RepresentationUtils::IsUnboxedInteger(rep)) { if (rep == kUnboxedUint8 || rep == kUnboxedInt8) { From 8f89bbe1a97b9941af1ab821522b343f47eaf92f Mon Sep 17 00:00:00 2001 From: Aravind Date: Fri, 13 Sep 2024 06:59:39 +0000 Subject: [PATCH 2/2] [vm/ffi] Not preventing the transformation for `InvalidExpression` in ffi Currently, we just "removed" the actual expression by transforming to `InvalidExpression('Invalid Type')` if a expression is Invalid. But this could cascade `.address` position errors (i.e transformer no longer able to walk `.address.abc` if the expression is just replaced with empty expression). So unwrapping actual expression from `InvalidExpression` and trying to transforming that actual expression through a recursive call. TEST=tests/ffi/static_checks/address_position_cascade_test.dart Bug: https://github.com/dart-lang/sdk/issues/56613 Change-Id: Ib1ba1d5021797b645c29ac296752d844d0935964 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/384080 Reviewed-by: Daco Harkes Reviewed-by: Slava Egorov Commit-Queue: Slava Egorov --- .../transformations/ffi/use_sites.dart | 22 +++--- ...art => address_position_cascade_test.dart} | 72 +++++++++++++++---- 2 files changed, 69 insertions(+), 25 deletions(-) rename tests/ffi/static_checks/{regress_56462_test.dart => address_position_cascade_test.dart} (65%) diff --git a/pkg/vm/lib/modular/transformations/ffi/use_sites.dart b/pkg/vm/lib/modular/transformations/ffi/use_sites.dart index 1c92a52c06da..96dfb9e7510e 100644 --- a/pkg/vm/lib/modular/transformations/ffi/use_sites.dart +++ b/pkg/vm/lib/modular/transformations/ffi/use_sites.dart @@ -1481,11 +1481,10 @@ mixin _FfiUseSiteTransformer on FfiTransformer { parent.addProcedure(newTarget); } } - return StaticInvocation( newTarget, Arguments(newArguments), - ); + )..parent = parent; } /// Converts a single parameter with argument for [_replaceNativeCall]. @@ -1510,16 +1509,19 @@ mixin _FfiUseSiteTransformer on FfiTransformer { return ('', parameterType, argument); } - // If an argument is an invalid expression, ffi don't need to report - // any error further, so skipping transformation for its descendants of the - // argument by transforming into empty expression (which is invalid) - if ((argument is AsExpression && argument.operand is InvalidExpression) || - argument is InvalidExpression) { - return ('E', parameterType, InvalidExpression('Invalid Type')); + if (argument is InvalidExpression && argument.expression is AsExpression) { + final parent = argument.expression as AsExpression; + final (_, _, transformedArgument) = + _replaceNativeCallParameterAndArgument( + parameter, parameterType, parent.operand, fileOffset); + return ( + 'E', + parameterType, + InvalidExpression('Invalid Type', transformedArgument) + ); } if (argument is InstanceInvocation && - argument.interfaceTarget == castMethod && - argument.functionType.returnType == parameterType) { + argument.interfaceTarget == castMethod) { // Argument is .address.cast(), so truncating .cast() final subExpression = argument.receiver; return _replaceNativeCallParameterAndArgument( diff --git a/tests/ffi/static_checks/regress_56462_test.dart b/tests/ffi/static_checks/address_position_cascade_test.dart similarity index 65% rename from tests/ffi/static_checks/regress_56462_test.dart rename to tests/ffi/static_checks/address_position_cascade_test.dart index 28cb24ea26d0..d7d39b0324cb 100644 --- a/tests/ffi/static_checks/regress_56462_test.dart +++ b/tests/ffi/static_checks/address_position_cascade_test.dart @@ -5,6 +5,9 @@ import 'dart:ffi'; import 'dart:typed_data'; +@Native)>() +external void myNonLeafNative(Pointer buff); + @Native, Pointer)>(isLeaf: true) external void myNativeWith2Param(Pointer buffer, Pointer buffer2); @@ -13,7 +16,7 @@ external void myNativeWith2Param(Pointer buffer, Pointer buffer2); external void myNativeWith3Param( Pointer buffer, Pointer buffer2, Pointer buffer3); -void test_wrong_type() { +void testDefinedLeaf() { final buffer = Int8List.fromList([1]); myNativeWith2Param(buffer.address, buffer.address); // ^^^^^^^ @@ -105,30 +108,69 @@ void test_wrong_type() { // [analyzer] COMPILE_TIME_ERROR.ARGUMENT_TYPE_NOT_ASSIGNABLE } -void test_undefined_arguments() { +void testUndefinedLeaf() { final buffer = Int8List.fromList([1]); - myNativeWith2Param(buffer.address.cast(), buffer.address.cd); - // ^^ - // [cfe] The getter 'cd' isn't defined for the class 'Pointer'. + myNativeWith2Param(buffer.address.cast(), buffer.address.doesntExist); + // ^^^^^^^^^^^ + // [cfe] The getter 'doesntExist' isn't defined for the class 'Pointer'. // [analyzer] COMPILE_TIME_ERROR.UNDEFINED_GETTER // ^^^^^^^ + // [cfe] The '.address' expression can only be used as argument to a leaf native external call. // [analyzer] COMPILE_TIME_ERROR.ADDRESS_POSITION - // This address position error is not expected which is a bug, - // https://github.com/dart-lang/sdk/issues/56613 - - myNativeWith2Param(buffer.address.cast().cd, buffer.address); - // ^^ - // [cfe] The getter 'cd' isn't defined for the class 'Pointer'. + myNativeWith2Param(buffer.address.cast().doesntExist, buffer.address); + // ^^^^^^^^^^^ + // [cfe] The getter 'doesntExist' isn't defined for the class 'Pointer'. // [analyzer] COMPILE_TIME_ERROR.UNDEFINED_GETTER // ^^^^^^^ + // [cfe] The '.address' expression can only be used as argument to a leaf native external call. + // [analyzer] COMPILE_TIME_ERROR.ADDRESS_POSITION +} + +void testUndefinedNonLeaf() { + final buffer = Int8List.fromList([1]); + + myNonLeafNative(buffer.address.cast().doesntExist); + // ^^^^^^^^^^^ + // [analyzer] COMPILE_TIME_ERROR.UNDEFINED_GETTER + // [cfe] The getter 'doesntExist' isn't defined for the class 'Pointer'. + // ^^^^^^^ + // [analyzer] COMPILE_TIME_ERROR.ADDRESS_POSITION + // [cfe] The '.address' expression can only be used as argument to a leaf native external call. + myNonLeafNative(buffer.address.doesntExist); + // ^^^^^^^^^^^ + // [analyzer] COMPILE_TIME_ERROR.UNDEFINED_GETTER + // [cfe] The getter 'doesntExist' isn't defined for the class 'Pointer'. + // ^^^^^^^ // [analyzer] COMPILE_TIME_ERROR.ADDRESS_POSITION + // [cfe] The '.address' expression can only be used as argument to a leaf native external call. +} - // This address position error is not expected which is a bug, - // https://github.com/dart-lang/sdk/issues/56613 +void testDefinedNonLeaf() { + final buffer = Int8List.fromList([1]); + myNonLeafNative(buffer.address.cast().address); + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + // [analyzer] COMPILE_TIME_ERROR.ARGUMENT_TYPE_NOT_ASSIGNABLE + // ^^^^^^^ + // [cfe] The argument type 'int' can't be assigned to the parameter type 'Pointer'. + // ^^^^^^^ + // [analyzer] COMPILE_TIME_ERROR.ADDRESS_POSITION + // [cfe] The '.address' expression can only be used as argument to a leaf native external call. + + myNonLeafNative(buffer.address.address); + // ^^^^^^^^^^^^^^^^^^^^^^ + // [analyzer] COMPILE_TIME_ERROR.ARGUMENT_TYPE_NOT_ASSIGNABLE + // ^^^^^^^ + // [cfe] The argument type 'int' can't be assigned to the parameter type 'Pointer'. + // ^^^^^^^ + // [analyzer] COMPILE_TIME_ERROR.ADDRESS_POSITION + // [cfe] The '.address' expression can only be used as argument to a leaf native external call. } void main() { - test_undefined_arguments(); - test_wrong_type(); + testDefinedLeaf(); + testDefinedNonLeaf(); + + testUndefinedLeaf(); + testUndefinedNonLeaf(); }