From a9da0ce73565a5b4357548686590cd0ae3735f68 Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Tue, 24 Oct 2017 10:39:25 +0200 Subject: [PATCH] [turbofan] Properly handle smis in monomorphic loads/stores. When lowering a monomorphic load/store, where multiple receiver maps have been recorded, but the action to be performed is the same (i.e. yielding undefined because the property is not found), TurboFan used to ignore the Smi case, leading to a pretty terrible deoptimization loop, as the LOAD_IC/STORE_IC properly recorded that state and thus didn't change it's state. Fixing this issue gives a 18-20% boost on the prettier test of the web-tooling-benchmark, which was suffering a lot from this problem. Bug: v8:6936, v8:6991 Change-Id: Id208ec7129a7f6b190d989bda31f936040393226 Reviewed-on: https://chromium-review.googlesource.com/735342 Reviewed-by: Michael Stanton Commit-Queue: Benedikt Meurer Cr-Commit-Position: refs/heads/master@{#48865} --- .../js-native-context-specialization.cc | 30 ++++++++++++++++--- test/mjsunit/regress/regress-6991.js | 17 +++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 test/mjsunit/regress/regress-6991.js diff --git a/src/compiler/js-native-context-specialization.cc b/src/compiler/js-native-context-specialization.cc index 971db97452..5307eea621 100644 --- a/src/compiler/js-native-context-specialization.cc +++ b/src/compiler/js-native-context-specialization.cc @@ -741,10 +741,32 @@ Reduction JSNativeContextSpecialization::ReduceNamedAccess( &receiver, &effect, control) && !access_builder.TryBuildNumberCheck(access_info.receiver_maps(), &receiver, &effect, control)) { - receiver = - access_builder.BuildCheckHeapObject(receiver, &effect, control); - access_builder.BuildCheckMaps(receiver, &effect, control, - access_info.receiver_maps()); + if (HasNumberMaps(access_info.receiver_maps())) { + // We need to also let Smi {receiver}s through in this case, so + // we construct a diamond, guarded by the Sminess of the {receiver} + // and if {receiver} is not a Smi just emit a sequence of map checks. + Node* check = graph()->NewNode(simplified()->ObjectIsSmi(), receiver); + Node* branch = graph()->NewNode(common()->Branch(), check, control); + + Node* if_true = graph()->NewNode(common()->IfTrue(), branch); + Node* etrue = effect; + + Node* if_false = graph()->NewNode(common()->IfFalse(), branch); + Node* efalse = effect; + { + access_builder.BuildCheckMaps(receiver, &efalse, if_false, + access_info.receiver_maps()); + } + + control = graph()->NewNode(common()->Merge(2), if_true, if_false); + effect = + graph()->NewNode(common()->EffectPhi(2), etrue, efalse, control); + } else { + receiver = + access_builder.BuildCheckHeapObject(receiver, &effect, control); + access_builder.BuildCheckMaps(receiver, &effect, control, + access_info.receiver_maps()); + } } // Generate the actual property access. diff --git a/test/mjsunit/regress/regress-6991.js b/test/mjsunit/regress/regress-6991.js new file mode 100644 index 0000000000..1c6b976977 --- /dev/null +++ b/test/mjsunit/regress/regress-6991.js @@ -0,0 +1,17 @@ +// Copyright 2017 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 --opt + +function foo(o) { return o.x; } + +assertEquals(undefined, foo({})); +assertEquals(undefined, foo(1)); +assertEquals(undefined, foo({})); +assertEquals(undefined, foo(1)); +%OptimizeFunctionOnNextCall(foo); +assertEquals(undefined, foo({})); +assertOptimized(foo); +assertEquals(undefined, foo(1)); +assertOptimized(foo);