From 22dd6dc2a6e1d426491037d5eeddb359d3570fe5 Mon Sep 17 00:00:00 2001 From: mstarzinger Date: Tue, 17 Feb 2015 08:37:24 -0800 Subject: [PATCH] Fix representation for CompareIC in JSGenericLowering. R=jarin@chromium.org TEST=mjsunit/regress/regress-3884 BUG=v8:3884 LOG=N Review URL: https://codereview.chromium.org/933913002 Cr-Commit-Position: refs/heads/master@{#26702} --- src/compiler/js-generic-lowering.cc | 53 ++++++++++++++++++++---- src/compiler/linkage.cc | 1 - src/runtime/runtime-object.cc | 29 ------------- src/runtime/runtime.h | 2 - test/cctest/compiler/test-run-jscalls.cc | 20 --------- test/cctest/compiler/test-run-jsops.cc | 2 +- test/mjsunit/mjsunit.status | 3 -- test/mjsunit/regress/regress-3884.js | 27 ++++++++++++ 8 files changed, 74 insertions(+), 63 deletions(-) create mode 100644 test/mjsunit/regress/regress-3884.js diff --git a/src/compiler/js-generic-lowering.cc b/src/compiler/js-generic-lowering.cc index 5ff3c718c1..0483c72c71 100644 --- a/src/compiler/js-generic-lowering.cc +++ b/src/compiler/js-generic-lowering.cc @@ -130,6 +130,8 @@ void JSGenericLowering::ReplaceWithCompareIC(Node* node, Token::Value token) { CallDescriptor* desc_compare = Linkage::GetStubCallDescriptor( isolate(), zone(), callable.descriptor(), 0, CallDescriptor::kPatchableCallSiteWithNop | FlagsForNode(node)); + + // Create a new call node asking a CompareIC for help. NodeVector inputs(zone()); inputs.reserve(node->InputCount() + 1); inputs.push_back(jsgraph()->HeapConstant(callable.code())); @@ -153,16 +155,53 @@ void JSGenericLowering::ReplaceWithCompareIC(Node* node, Token::Value token) { Node* compare = graph()->NewNode(common()->Call(desc_compare), static_cast(inputs.size()), &inputs.front()); + NodeProperties::SetBounds( + compare, Bounds(Type::None(zone()), Type::UntaggedSigned(zone()))); - node->ReplaceInput(0, compare); - node->ReplaceInput(1, jsgraph()->SmiConstant(token)); - - if (has_frame_state) { - // Remove the frame state from inputs. - node->RemoveInput(NodeProperties::FirstFrameStateIndex(node)); + // Decide how the return value from the above CompareIC can be converted into + // a JavaScript boolean oddball depending on the given token. + Node* false_value = jsgraph()->FalseConstant(); + Node* true_value = jsgraph()->TrueConstant(); + const Operator* op = nullptr; + switch (token) { + case Token::EQ: // a == 0 + case Token::EQ_STRICT: + op = machine()->WordEqual(); + break; + case Token::NE: // a != 0 becomes !(a == 0) + case Token::NE_STRICT: + op = machine()->WordEqual(); + std::swap(true_value, false_value); + break; + case Token::LT: // a < 0 + op = machine()->IntLessThan(); + break; + case Token::GT: // a > 0 becomes !(a <= 0) + op = machine()->IntLessThanOrEqual(); + std::swap(true_value, false_value); + break; + case Token::LTE: // a <= 0 + op = machine()->IntLessThanOrEqual(); + break; + case Token::GTE: // a >= 0 becomes !(a < 0) + op = machine()->IntLessThan(); + std::swap(true_value, false_value); + break; + default: + UNREACHABLE(); } + Node* booleanize = graph()->NewNode(op, compare, jsgraph()->ZeroConstant()); - ReplaceWithRuntimeCall(node, Runtime::kBooleanize); + // Finally patch the original node to select a boolean. + NodeProperties::ReplaceWithValue(node, node, compare); + // TODO(mstarzinger): Just a work-around because SelectLowering might + // otherwise introduce a Phi without any uses, making Scheduler unhappy. + if (node->UseCount() == 0) return; + node->TrimInputCount(3); + node->ReplaceInput(0, booleanize); + node->ReplaceInput(1, true_value); + node->ReplaceInput(2, false_value); + PatchOperator(node, common()->Select(kMachAnyTagged)); } diff --git a/src/compiler/linkage.cc b/src/compiler/linkage.cc index b47130f0f7..eedf9ed746 100644 --- a/src/compiler/linkage.cc +++ b/src/compiler/linkage.cc @@ -104,7 +104,6 @@ bool Linkage::NeedsFrameState(Runtime::FunctionId function) { // not to call into arbitrary JavaScript, not to throw, and not to deoptimize // are blacklisted here and can be called without a FrameState. switch (function) { - case Runtime::kBooleanize: case Runtime::kDeclareGlobals: // TODO(jarin): Is it safe? case Runtime::kDefineClassMethod: // TODO(jarin): Is it safe? case Runtime::kDefineGetterPropertyUnchecked: // TODO(jarin): Is it safe? diff --git a/src/runtime/runtime-object.cc b/src/runtime/runtime-object.cc index 844c633bdb..96d9331038 100644 --- a/src/runtime/runtime-object.cc +++ b/src/runtime/runtime-object.cc @@ -1197,35 +1197,6 @@ RUNTIME_FUNCTION(Runtime_Typeof) { } -RUNTIME_FUNCTION(Runtime_Booleanize) { - SealHandleScope shs(isolate); - DCHECK(args.length() == 2); - CONVERT_ARG_CHECKED(Object, value_raw, 0); - CONVERT_SMI_ARG_CHECKED(token_raw, 1); - intptr_t value = reinterpret_cast(value_raw); - Token::Value token = static_cast(token_raw); - switch (token) { - case Token::EQ: - case Token::EQ_STRICT: - return isolate->heap()->ToBoolean(value == 0); - case Token::NE: - case Token::NE_STRICT: - return isolate->heap()->ToBoolean(value != 0); - case Token::LT: - return isolate->heap()->ToBoolean(value < 0); - case Token::GT: - return isolate->heap()->ToBoolean(value > 0); - case Token::LTE: - return isolate->heap()->ToBoolean(value <= 0); - case Token::GTE: - return isolate->heap()->ToBoolean(value >= 0); - default: - // This should only happen during natives fuzzing. - return isolate->heap()->undefined_value(); - } -} - - RUNTIME_FUNCTION(Runtime_NewStringWrapper) { HandleScope scope(isolate); DCHECK(args.length() == 1); diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h index db2117bdd0..1ea85c83ab 100644 --- a/src/runtime/runtime.h +++ b/src/runtime/runtime.h @@ -98,8 +98,6 @@ namespace internal { F(ToBool, 1, 1) \ F(Typeof, 1, 1) \ \ - F(Booleanize, 2, 1) /* TODO(turbofan): Only temporary */ \ - \ F(StringToNumber, 1, 1) \ F(StringParseInt, 2, 1) \ F(StringParseFloat, 1, 1) \ diff --git a/test/cctest/compiler/test-run-jscalls.cc b/test/cctest/compiler/test-run-jscalls.cc index dec7194c4a..43ab38b8c2 100644 --- a/test/cctest/compiler/test-run-jscalls.cc +++ b/test/cctest/compiler/test-run-jscalls.cc @@ -184,26 +184,6 @@ TEST(RuntimeCallInline) { } -TEST(RuntimeCallBooleanize) { - // TODO(turbofan): %Booleanize will disappear, don't hesitate to remove this - // test case, two-argument case is covered by the above test already. - FLAG_allow_natives_syntax = true; - FunctionTester T("(function(a,b) { return %Booleanize(a, b); })"); - - T.CheckCall(T.true_value(), T.Val(-1), T.Val(Token::LT)); - T.CheckCall(T.false_value(), T.Val(-1), T.Val(Token::EQ)); - T.CheckCall(T.false_value(), T.Val(-1), T.Val(Token::GT)); - - T.CheckCall(T.false_value(), T.Val(0.0), T.Val(Token::LT)); - T.CheckCall(T.true_value(), T.Val(0.0), T.Val(Token::EQ)); - T.CheckCall(T.false_value(), T.Val(0.0), T.Val(Token::GT)); - - T.CheckCall(T.false_value(), T.Val(1), T.Val(Token::LT)); - T.CheckCall(T.false_value(), T.Val(1), T.Val(Token::EQ)); - T.CheckCall(T.true_value(), T.Val(1), T.Val(Token::GT)); -} - - TEST(EvalCall) { FunctionTester T("(function(a,b) { return eval(a); })"); Handle g(T.function->context()->global_object()->global_proxy()); diff --git a/test/cctest/compiler/test-run-jsops.cc b/test/cctest/compiler/test-run-jsops.cc index df65afa6c5..bb7c239a59 100644 --- a/test/cctest/compiler/test-run-jsops.cc +++ b/test/cctest/compiler/test-run-jsops.cc @@ -212,7 +212,7 @@ TEST(BinopLessThan) { } -TEST(BinopLessThanEqual) { +TEST(BinopLessThanOrEqual) { FunctionTester T("(function(a,b) { return a <= b; })"); T.CheckTrue(7, 8); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index ed3505a35b..dc96a1de35 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -298,9 +298,6 @@ # BUG(v8:3457). 'deserialize-reference': [PASS, FAIL], - # BUG(v8:3884). - 'regress/regress-builtinbust-7': [PASS, ['gc_stress == True', SKIP]], - # Slow tests. 'array-concat': [PASS, SLOW], 'array-constructor': [PASS, SLOW], diff --git a/test/mjsunit/regress/regress-3884.js b/test/mjsunit/regress/regress-3884.js new file mode 100644 index 0000000000..ecd000f6c7 --- /dev/null +++ b/test/mjsunit/regress/regress-3884.js @@ -0,0 +1,27 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax --expose-gc + +function f(x) { + // TurboFan will hoist the CompareIC for x === 'some_string' and spill it. + if (x === 'some_other_string_1' || x === 'some_string') { + gc(); + } + if (x === 'some_other_string_2' || x === 'some_string') { + gc(); + } + // TurboFan will hoist the CompareIC for x === 1.4 and spill it. + if (x === 1.7 || x === 1.4) { + gc(); + } + if (x === 1.9 || x === 1.4) { + gc(); + } +} + +%OptimizeFunctionOnNextCall(f); + +f('some_other_string_1'); +f(1.7);