From cc89eb80241dd207eb0e3ff42a887afee682aba9 Mon Sep 17 00:00:00 2001 From: Leszek Swirski Date: Fri, 16 Dec 2022 12:25:06 +0100 Subject: [PATCH] [ic] Fix no-feedback binops Fix a non-terminating recursive call in BitwiseAnd/Or/Xor to instead perform the truncation + bitwise op directly. Also, fix up Generate_BitwiseBinaryOpWithOptionalFeedback to allow passing in optional feedback. This is a drive-by from attempting to fix the above issue by calling Generate_BitwiseBinaryOp. Bug: v8:9407 Change-Id: I2f91779de5533d1911b408f80664a4c9ae5c7342 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4111545 Commit-Queue: Jakob Kummerow Reviewed-by: Jakob Kummerow Auto-Submit: Leszek Swirski Cr-Commit-Position: refs/heads/main@{#84900} --- src/builtins/number.tq | 17 ++++++++----- src/codegen/code-stub-assembler.cc | 9 ------- src/codegen/code-stub-assembler.h | 14 +++------- src/ic/binary-op-assembler.cc | 41 +++++++++++++++++------------- 4 files changed, 38 insertions(+), 43 deletions(-) diff --git a/src/builtins/number.tq b/src/builtins/number.tq index f88466bc9f..f9cc7dd8f4 100644 --- a/src/builtins/number.tq +++ b/src/builtins/number.tq @@ -311,8 +311,6 @@ transitioning javascript builtin NumberParseInt( } extern builtin NonNumberToNumeric(implicit context: Context)(JSAny): Numeric; -extern builtin BitwiseAnd(implicit context: Context)(Number, Number): Number; -extern builtin BitwiseXor(implicit context: Context)(Number, Number): Number; extern builtin Subtract(implicit context: Context)(Number, Number): Number; extern builtin Add(implicit context: Context)(Number, Number): Number; extern builtin StringAddConvertLeft(implicit context: Context)( @@ -323,6 +321,7 @@ extern builtin StringAddConvertRight(implicit context: Context)( extern macro BitwiseOp(int32, int32, constexpr Operation): Number; extern macro RelationalComparison( constexpr Operation, JSAny, JSAny, Context): Boolean; +extern macro TruncateNumberToWord32(Number): int32; // TODO(bbudge) Use a simpler macro structure that doesn't loop when converting // non-numbers, if such a code sequence doesn't make the builtin bigger. @@ -699,7 +698,7 @@ builtin BitwiseNot(implicit context: Context)(value: JSAny): Numeric { try { UnaryOp1(value) otherwise Number, BigInt; } label Number(n: Number) { - tail BitwiseXor(n, -1); + return BitwiseOp(TruncateNumberToWord32(n), -1, Operation::kBitwiseXor); } label BigInt(b: BigInt) { return runtime::BigIntUnaryOp( context, b, SmiTag(Operation::kBitwiseNot)); @@ -754,7 +753,9 @@ builtin BitwiseAnd(implicit context: Context)( try { BinaryOp1(left, right) otherwise Number, AtLeastOneBigInt; } label Number(left: Number, right: Number) { - tail BitwiseAnd(left, right); + return BitwiseOp( + TruncateNumberToWord32(left), TruncateNumberToWord32(right), + Operation::kBitwiseAnd); } label AtLeastOneBigInt(left: Numeric, right: Numeric) { tail bigint::BigIntBitwiseAnd(left, right); } @@ -765,7 +766,9 @@ builtin BitwiseOr(implicit context: Context)( try { BinaryOp1(left, right) otherwise Number, AtLeastOneBigInt; } label Number(left: Number, right: Number) { - tail BitwiseOr(left, right); + return BitwiseOp( + TruncateNumberToWord32(left), TruncateNumberToWord32(right), + Operation::kBitwiseOr); } label AtLeastOneBigInt(left: Numeric, right: Numeric) { tail bigint::BigIntBitwiseOr(left, right); } @@ -776,7 +779,9 @@ builtin BitwiseXor(implicit context: Context)( try { BinaryOp1(left, right) otherwise Number, AtLeastOneBigInt; } label Number(left: Number, right: Number) { - tail BitwiseXor(left, right); + return BitwiseOp( + TruncateNumberToWord32(left), TruncateNumberToWord32(right), + Operation::kBitwiseXor); } label AtLeastOneBigInt(left: Numeric, right: Numeric) { tail bigint::BigIntBitwiseXor(left, right); } diff --git a/src/codegen/code-stub-assembler.cc b/src/codegen/code-stub-assembler.cc index 23111e605f..a8a69133a0 100644 --- a/src/codegen/code-stub-assembler.cc +++ b/src/codegen/code-stub-assembler.cc @@ -7982,15 +7982,6 @@ TNode CodeStubAssembler::ToBigIntConvertNumber(TNode context, return var_result.value(); } -void CodeStubAssembler::TaggedToBigIntWithFeedback( - TNode context, TNode value, Label* if_not_bigint, - Label* if_bigint, Label* if_bigint64, TVariable* var_bigint, - TVariable* var_feedback) { - DCHECK_NOT_NULL(var_feedback); - TaggedToBigInt(context, value, if_not_bigint, if_bigint, if_bigint64, - var_bigint, var_feedback); -} - void CodeStubAssembler::TaggedToBigInt(TNode context, TNode value, Label* if_not_bigint, Label* if_bigint, diff --git a/src/codegen/code-stub-assembler.h b/src/codegen/code-stub-assembler.h index f7cdced051..e67e7ad5ef 100644 --- a/src/codegen/code-stub-assembler.h +++ b/src/codegen/code-stub-assembler.h @@ -2504,11 +2504,10 @@ class V8_EXPORT_PRIVATE CodeStubAssembler TNode ChangeBoolToInt32(TNode b); - void TaggedToBigIntWithFeedback(TNode context, TNode value, - Label* if_not_bigint, Label* if_bigint, - Label* if_bigint64, - TVariable* var_bigint, - TVariable* var_feedback); + void TaggedToBigInt(TNode context, TNode value, + Label* if_not_bigint, Label* if_bigint, + Label* if_bigint64, TVariable* var_bigint, + TVariable* var_feedback); // Ensures that {var_shared_value} is shareable across Isolates, and throws if // not. @@ -4320,11 +4319,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler TNode context, TNode input, Object::Conversion mode, BigIntHandling bigint_handling = BigIntHandling::kThrow); - void TaggedToBigInt(TNode context, TNode value, - Label* if_not_bigint, Label* if_bigint, - Label* if_bigint64, TVariable* var_bigint, - TVariable* var_feedback); - enum IsKnownTaggedPointer { kNo, kYes }; template void TaggedToWord32OrBigIntImpl(TNode context, TNode value, diff --git a/src/ic/binary-op-assembler.cc b/src/ic/binary-op-assembler.cc index 212d49237a..a1dfbe49d8 100644 --- a/src/ic/binary-op-assembler.cc +++ b/src/ic/binary-op-assembler.cc @@ -840,15 +840,14 @@ TNode BinaryOpAssembler::Generate_BitwiseBinaryOpWithOptionalFeedback( Label if_bigint_mix(this, Label::kDeferred); BIND(&if_left_bigint); - TaggedToBigIntWithFeedback(context(), right, &if_bigint_mix, - &if_both_bigint, nullptr, &var_right_bigint, - &var_right_feedback); + TaggedToBigInt(context(), right, &if_bigint_mix, &if_both_bigint, nullptr, + &var_right_bigint, slot ? &var_right_feedback : nullptr); if (IsBigInt64OpSupported(this, bitwise_op)) { BIND(&if_left_bigint64); - TaggedToBigIntWithFeedback(context(), right, &if_bigint_mix, - &if_both_bigint, &if_both_bigint64, - &var_right_bigint, &var_right_feedback); + TaggedToBigInt(context(), right, &if_bigint_mix, &if_both_bigint, + &if_both_bigint64, &var_right_bigint, + slot ? &var_right_feedback : nullptr); BIND(&if_both_bigint64); if (slot) { @@ -907,10 +906,12 @@ TNode BinaryOpAssembler::Generate_BitwiseBinaryOpWithOptionalFeedback( // Check for sentinel that signals BigIntTooBig exception. GotoIfNot(TaggedIsSmi(result.value()), &done); - // Update feedback to prevent deopt loop. - UpdateFeedback(SmiConstant(BinaryOperationFeedback::kAny), - (*maybe_feedback_vector)(), *slot, - update_feedback_mode); + if (slot) { + // Update feedback to prevent deopt loop. + UpdateFeedback(SmiConstant(BinaryOperationFeedback::kAny), + (*maybe_feedback_vector)(), *slot, + update_feedback_mode); + } ThrowRangeError(context(), MessageTemplate::kBigIntTooBig); break; } @@ -921,10 +922,12 @@ TNode BinaryOpAssembler::Generate_BitwiseBinaryOpWithOptionalFeedback( // Check for sentinel that signals BigIntTooBig exception. GotoIfNot(TaggedIsSmi(result.value()), &done); - // Update feedback to prevent deopt loop. - UpdateFeedback(SmiConstant(BinaryOperationFeedback::kAny), - (*maybe_feedback_vector)(), *slot, - update_feedback_mode); + if (slot) { + // Update feedback to prevent deopt loop. + UpdateFeedback(SmiConstant(BinaryOperationFeedback::kAny), + (*maybe_feedback_vector)(), *slot, + update_feedback_mode); + } ThrowRangeError(context(), MessageTemplate::kBigIntTooBig); break; } @@ -935,10 +938,12 @@ TNode BinaryOpAssembler::Generate_BitwiseBinaryOpWithOptionalFeedback( // Check for sentinel that signals BigIntTooBig exception. GotoIfNot(TaggedIsSmi(result.value()), &done); - // Update feedback to prevent deopt loop. - UpdateFeedback(SmiConstant(BinaryOperationFeedback::kAny), - (*maybe_feedback_vector)(), *slot, - update_feedback_mode); + if (slot) { + // Update feedback to prevent deopt loop. + UpdateFeedback(SmiConstant(BinaryOperationFeedback::kAny), + (*maybe_feedback_vector)(), *slot, + update_feedback_mode); + } ThrowRangeError(context(), MessageTemplate::kBigIntTooBig); break; }