From 630fbf672a987cfda7c07ea629a29644640bfe3e Mon Sep 17 00:00:00 2001 From: Santiago Aboy Solanes Date: Mon, 28 Sep 2020 13:47:22 +0100 Subject: [PATCH] [CSA] Create a PlainPrimitiveToNumberBuiltin Previously ToNumber could be called with an empty context. In a previous CL (https://crrev.com/c/v8/v8/+/2078580) we added DCHECKS to make sure that some paths were not using the empty context. Now we are doing the next step of adding a primitive to separate the cases. Small update from delphick@ to get the builtin descriptor right. Bug: v8:6949, v8:10933 Change-Id: Ie40b169f680f60a08eb26fac1fcfcef7d6169e65 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2428863 Commit-Queue: Santiago Aboy Solanes Reviewed-by: Ross McIlroy Cr-Commit-Position: refs/heads/master@{#70162} --- src/builtins/builtins-conversion-gen.cc | 11 +-- src/builtins/builtins-definitions.h | 1 + src/codegen/code-stub-assembler.cc | 90 ++++++++++++++++++++++--- src/codegen/code-stub-assembler.h | 12 +++- src/codegen/interface-descriptors.cc | 6 ++ src/codegen/interface-descriptors.h | 8 +++ src/compiler/graph-assembler.cc | 9 +-- src/compiler/graph-assembler.h | 1 + src/compiler/js-graph.cc | 3 + src/compiler/js-graph.h | 1 + 10 files changed, 120 insertions(+), 22 deletions(-) diff --git a/src/builtins/builtins-conversion-gen.cc b/src/builtins/builtins-conversion-gen.cc index 54fa752969..cf56c5366c 100644 --- a/src/builtins/builtins-conversion-gen.cc +++ b/src/builtins/builtins-conversion-gen.cc @@ -14,15 +14,18 @@ namespace internal { // ES6 section 7.1.3 ToNumber ( argument ) TF_BUILTIN(ToNumber, CodeStubAssembler) { - // TODO(solanes, v8:6949): Changing this to a TNode crashes with the - // empty context. Context might not be needed, but it is propagated all over - // the place and hard to pull out. - Node* context = Parameter(Descriptor::kContext); + TNode context = CAST(Parameter(Descriptor::kContext)); TNode input = CAST(Parameter(Descriptor::kArgument)); Return(ToNumber(context, input)); } +TF_BUILTIN(PlainPrimitiveToNumber, CodeStubAssembler) { + TNode input = CAST(Parameter(Descriptor::kArgument)); + + Return(PlainPrimitiveToNumber(input)); +} + // Like ToNumber, but also converts BigInts. TF_BUILTIN(ToNumberConvertBigInt, CodeStubAssembler) { TNode context = CAST(Parameter(Descriptor::kContext)); diff --git a/src/builtins/builtins-definitions.h b/src/builtins/builtins-definitions.h index 336437a104..511e748acd 100644 --- a/src/builtins/builtins-definitions.h +++ b/src/builtins/builtins-definitions.h @@ -189,6 +189,7 @@ namespace internal { \ /* Type conversions */ \ TFC(ToNumber, TypeConversion) \ + TFC(PlainPrimitiveToNumber, TypeConversionNoContext) \ TFC(ToNumberConvertBigInt, TypeConversion) \ TFC(Typeof, Typeof) \ TFC(GetSuperConstructor, Typeof) \ diff --git a/src/codegen/code-stub-assembler.cc b/src/codegen/code-stub-assembler.cc index 86877949d7..9d9b6658b8 100644 --- a/src/codegen/code-stub-assembler.cc +++ b/src/codegen/code-stub-assembler.cc @@ -6755,6 +6755,12 @@ TNode CodeStubAssembler::NumberToString(TNode input) { return result.value(); } +// TODO(solanes, v8:6949): Refactor this to check for JSReceivers first. If we +// have a JSReceiver, extract the primitive and fallthrough. Otherwise, continue +// asking for the other instance types. This will make it so that we can remove +// the loop (which was looping at most once). Also, see if we can make use of +// PlainPrimitiveNonNumberToNumber to de-duplicate code, maybe changing it to a +// TryPlainPrimitiveNonNumberToNumber with a Label* as a parameter. TNode CodeStubAssembler::NonNumberToNumberOrNumeric( TNode context, TNode input, Object::Conversion mode, BigIntHandling bigint_handling) { @@ -6792,7 +6798,6 @@ TNode CodeStubAssembler::NonNumberToNumberOrNumeric( } BIND(&if_inputisbigint); - CSA_ASSERT(this, Word32And(TaggedIsNotSmi(context), IsContext(context))); if (mode == Object::Conversion::kToNumeric) { var_result = CAST(input); Goto(&end); @@ -6817,9 +6822,9 @@ TNode CodeStubAssembler::NonNumberToNumberOrNumeric( BIND(&if_inputisreceiver); { - CSA_ASSERT(this, Word32And(TaggedIsNotSmi(context), IsContext(context))); - // The {input} is a JSReceiver, we need to convert it to a Primitive first - // using the ToPrimitive type conversion, preferably yielding a Number. + // The {input} is a JSReceiver, we need to convert it to a Primitive + // first using the ToPrimitive type conversion, preferably yielding a + // Number. Callable callable = CodeFactory::NonPrimitiveToPrimitive( isolate(), ToPrimitiveHint::kNumber); TNode result = CallStub(callable, context, input); @@ -6832,15 +6837,16 @@ TNode CodeStubAssembler::NonNumberToNumberOrNumeric( BIND(&if_done); { - // The ToPrimitive conversion already gave us a Number/Numeric, so we're - // done. + // The ToPrimitive conversion already gave us a Number/Numeric, so + // we're done. var_result = CAST(result); Goto(&end); } BIND(&if_notdone); { - // We now have a Primitive {result}, but it's not yet a Number/Numeric. + // We now have a Primitive {result}, but it's not yet a + // Number/Numeric. var_input = CAST(result); Goto(&loop); } @@ -6848,7 +6854,6 @@ TNode CodeStubAssembler::NonNumberToNumberOrNumeric( BIND(&if_inputisother); { - CSA_ASSERT(this, Word32And(TaggedIsNotSmi(context), IsContext(context))); // The {input} is something else (e.g. Symbol), let the runtime figure // out the correct exception. // Note: We cannot tail call to the runtime here, as js-to-wasm @@ -6873,14 +6878,46 @@ TNode CodeStubAssembler::NonNumberToNumberOrNumeric( } TNode CodeStubAssembler::NonNumberToNumber( - SloppyTNode context, SloppyTNode input, + TNode context, SloppyTNode input, BigIntHandling bigint_handling) { return CAST(NonNumberToNumberOrNumeric( context, input, Object::Conversion::kToNumber, bigint_handling)); } +TNode CodeStubAssembler::PlainPrimitiveNonNumberToNumber( + TNode input) { + CSA_ASSERT(this, Word32BinaryNot(IsHeapNumber(input))); + TVARIABLE(Number, var_result); + Label done(this); + + // Dispatch on the {input} instance type. + TNode input_instance_type = LoadInstanceType(input); + Label if_inputisstring(this), if_inputisoddball(this); + GotoIf(IsStringInstanceType(input_instance_type), &if_inputisstring); + CSA_ASSERT(this, InstanceTypeEqual(input_instance_type, ODDBALL_TYPE)); + Goto(&if_inputisoddball); + + BIND(&if_inputisstring); + { + // The {input} is a String, use the fast stub to convert it to a Number. + TNode string_input = CAST(input); + var_result = StringToNumber(string_input); + Goto(&done); + } + + BIND(&if_inputisoddball); + { + // The {input} is an Oddball, we just need to load the Number value of it. + var_result = LoadObjectField(input, Oddball::kToNumberOffset); + Goto(&done); + } + + BIND(&done); + return var_result.value(); +} + TNode CodeStubAssembler::NonNumberToNumeric( - SloppyTNode context, SloppyTNode input) { + TNode context, SloppyTNode input) { return NonNumberToNumberOrNumeric(context, input, Object::Conversion::kToNumeric); } @@ -6909,7 +6946,7 @@ TNode CodeStubAssembler::ToNumber_Inline(SloppyTNode context, return var_result.value(); } -TNode CodeStubAssembler::ToNumber(SloppyTNode context, +TNode CodeStubAssembler::ToNumber(TNode context, SloppyTNode input, BigIntHandling bigint_handling) { TVARIABLE(Number, var_result); @@ -6942,6 +6979,37 @@ TNode CodeStubAssembler::ToNumber(SloppyTNode context, return var_result.value(); } +TNode CodeStubAssembler::PlainPrimitiveToNumber(TNode input) { + TVARIABLE(Number, var_result); + Label end(this); + + Label not_smi(this, Label::kDeferred); + GotoIfNot(TaggedIsSmi(input), ¬_smi); + TNode input_smi = CAST(input); + var_result = input_smi; + Goto(&end); + + BIND(¬_smi); + { + Label not_heap_number(this, Label::kDeferred); + TNode input_ho = CAST(input); + GotoIfNot(IsHeapNumber(input_ho), ¬_heap_number); + + TNode input_hn = CAST(input_ho); + var_result = input_hn; + Goto(&end); + + BIND(¬_heap_number); + { + var_result = PlainPrimitiveNonNumberToNumber(input_ho); + Goto(&end); + } + } + + BIND(&end); + return var_result.value(); +} + TNode CodeStubAssembler::ToBigInt(TNode context, TNode input) { TVARIABLE(BigInt, var_result); diff --git a/src/codegen/code-stub-assembler.h b/src/codegen/code-stub-assembler.h index c2da6a0df9..fd3d94f7cc 100644 --- a/src/codegen/code-stub-assembler.h +++ b/src/codegen/code-stub-assembler.h @@ -2464,20 +2464,23 @@ class V8_EXPORT_PRIVATE CodeStubAssembler // Convert a Non-Number object to a Number. TNode NonNumberToNumber( - SloppyTNode context, SloppyTNode input, + TNode context, SloppyTNode input, BigIntHandling bigint_handling = BigIntHandling::kThrow); // Convert a Non-Number object to a Numeric. - TNode NonNumberToNumeric(SloppyTNode context, + TNode NonNumberToNumeric(TNode context, SloppyTNode input); // Convert any object to a Number. // Conforms to ES#sec-tonumber if {bigint_handling} == kThrow. // With {bigint_handling} == kConvertToNumber, matches behavior of // tc39.github.io/proposal-bigint/#sec-number-constructor-number-value. TNode ToNumber( - SloppyTNode context, SloppyTNode input, + TNode context, SloppyTNode input, BigIntHandling bigint_handling = BigIntHandling::kThrow); TNode ToNumber_Inline(SloppyTNode context, SloppyTNode input); + // Convert any plain primitive to a Number. No need to handle BigInts since + // they are not plain primitives. + TNode PlainPrimitiveToNumber(TNode input); // Try to convert an object to a BigInt. Throws on failure (e.g. for Numbers). // https://tc39.github.io/proposal-bigint/#sec-to-bigint @@ -3655,6 +3658,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler TNode> array, TNode index, TNode value, WriteBarrierMode barrier_mode = UPDATE_WRITE_BARRIER, int additional_offset = 0); + + // Converts {input} to a number. {input} must be a plain primitve. + TNode PlainPrimitiveNonNumberToNumber(TNode input); }; class V8_EXPORT_PRIVATE CodeStubArguments { diff --git a/src/codegen/interface-descriptors.cc b/src/codegen/interface-descriptors.cc index 5854220b7b..8a6235fa08 100644 --- a/src/codegen/interface-descriptors.cc +++ b/src/codegen/interface-descriptors.cc @@ -287,6 +287,12 @@ void TypeConversionDescriptor::InitializePlatformSpecific( data->InitializePlatformSpecific(arraysize(registers), registers); } +void TypeConversionNoContextDescriptor::InitializePlatformSpecific( + CallInterfaceDescriptorData* data) { + Register registers[] = {TypeConversionDescriptor::ArgumentRegister()}; + data->InitializePlatformSpecific(arraysize(registers), registers); +} + void TypeConversionStackParameterDescriptor::InitializePlatformSpecific( CallInterfaceDescriptorData* data) { data->InitializePlatformSpecific(0, nullptr); diff --git a/src/codegen/interface-descriptors.h b/src/codegen/interface-descriptors.h index 726035b2dc..d307502276 100644 --- a/src/codegen/interface-descriptors.h +++ b/src/codegen/interface-descriptors.h @@ -93,6 +93,7 @@ namespace internal { V(StringAtAsString) \ V(StringSubstring) \ V(TypeConversion) \ + V(TypeConversionNoContext) \ V(TypeConversionStackParameter) \ V(Typeof) \ V(UnaryOp_WithFeedback) \ @@ -909,6 +910,13 @@ class TypeConversionDescriptor final : public CallInterfaceDescriptor { static const Register ArgumentRegister(); }; +class TypeConversionNoContextDescriptor final : public CallInterfaceDescriptor { + public: + DEFINE_PARAMETERS_NO_CONTEXT(kArgument) + DEFINE_PARAMETER_TYPES(MachineType::AnyTagged()) + DECLARE_DESCRIPTOR(TypeConversionNoContextDescriptor, CallInterfaceDescriptor) +}; + class TypeConversionStackParameterDescriptor final : public CallInterfaceDescriptor { public: diff --git a/src/compiler/graph-assembler.cc b/src/compiler/graph-assembler.cc index e3f5a22761..9f295773e8 100644 --- a/src/compiler/graph-assembler.cc +++ b/src/compiler/graph-assembler.cc @@ -716,9 +716,9 @@ Node* GraphAssembler::UnsafePointerAdd(Node* base, Node* external) { } TNode JSGraphAssembler::PlainPrimitiveToNumber(TNode value) { - return AddNode(graph()->NewNode(PlainPrimitiveToNumberOperator(), - ToNumberBuiltinConstant(), value, - NoContextConstant(), effect())); + return AddNode(graph()->NewNode( + PlainPrimitiveToNumberOperator(), PlainPrimitiveToNumberBuiltinConstant(), + value, effect())); } Node* GraphAssembler::BitcastWordToTaggedSigned(Node* value) { @@ -959,7 +959,8 @@ void GraphAssembler::InitializeEffectControl(Node* effect, Node* control) { Operator const* JSGraphAssembler::PlainPrimitiveToNumberOperator() { if (!to_number_operator_.is_set()) { - Callable callable = Builtins::CallableFor(isolate(), Builtins::kToNumber); + Callable callable = + Builtins::CallableFor(isolate(), Builtins::kPlainPrimitiveToNumber); CallDescriptor::Flags flags = CallDescriptor::kNoFlags; auto call_descriptor = Linkage::GetStubCallDescriptor( graph()->zone(), callable.descriptor(), diff --git a/src/compiler/graph-assembler.h b/src/compiler/graph-assembler.h index 7c948c82a1..17d7fe09d3 100644 --- a/src/compiler/graph-assembler.h +++ b/src/compiler/graph-assembler.h @@ -127,6 +127,7 @@ class BasicBlock; V(One, Number) \ V(TheHole, Oddball) \ V(ToNumberBuiltin, Code) \ + V(PlainPrimitiveToNumberBuiltin, Code) \ V(True, Boolean) \ V(Undefined, Oddball) \ V(Zero, Number) diff --git a/src/compiler/js-graph.cc b/src/compiler/js-graph.cc index 6b8d9761ff..120f8ee21d 100644 --- a/src/compiler/js-graph.cc +++ b/src/compiler/js-graph.cc @@ -129,6 +129,9 @@ DEFINE_GETTER(BooleanMapConstant, HeapConstant(factory()->boolean_map())) DEFINE_GETTER(ToNumberBuiltinConstant, HeapConstant(BUILTIN_CODE(isolate(), ToNumber))) +DEFINE_GETTER(PlainPrimitiveToNumberBuiltinConstant, + HeapConstant(BUILTIN_CODE(isolate(), PlainPrimitiveToNumber))) + DEFINE_GETTER(EmptyFixedArrayConstant, HeapConstant(factory()->empty_fixed_array())) diff --git a/src/compiler/js-graph.h b/src/compiler/js-graph.h index b055f399df..a17b615b3b 100644 --- a/src/compiler/js-graph.h +++ b/src/compiler/js-graph.h @@ -85,6 +85,7 @@ class V8_EXPORT_PRIVATE JSGraph : public MachineGraph { V(BigIntMapConstant) \ V(BooleanMapConstant) \ V(ToNumberBuiltinConstant) \ + V(PlainPrimitiveToNumberBuiltinConstant) \ V(EmptyFixedArrayConstant) \ V(EmptyStringConstant) \ V(FixedArrayMapConstant) \