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

@@ -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<int>(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));
 }
 
 

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?

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<intptr_t>(value_raw);
-  Token::Value token = static_cast<Token::Value>(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);

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)                                \

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<JSObject> g(T.function->context()->global_object()->global_proxy());

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);

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],

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);