From 9925079bb2b1ac1d9b0f17fa9a9e99d323488353 Mon Sep 17 00:00:00 2001 From: Maya Lekova Date: Wed, 28 Aug 2019 16:57:38 +0200 Subject: [PATCH] [turbofan] Remove JSGraph::Constant for Handles Bug: v8:7790 Change-Id: I666f545f4b5b7b5aeaed4ce2910240ef54f40c0e Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1773251 Commit-Queue: Maya Lekova Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#63427} --- src/compiler/allocation-builder-inl.h | 16 ++++--- src/compiler/allocation-builder.h | 8 +--- src/compiler/js-create-lowering.cc | 42 +++++++++++-------- src/compiler/js-graph.cc | 20 --------- src/compiler/js-graph.h | 8 +--- .../js-native-context-specialization.cc | 3 +- .../cctest/compiler/test-js-constant-cache.cc | 32 ++++++++++---- .../test-js-context-specialization.cc | 18 +++++--- 8 files changed, 73 insertions(+), 74 deletions(-) diff --git a/src/compiler/allocation-builder-inl.h b/src/compiler/allocation-builder-inl.h index 8da7c685a1..4cab0a7e6e 100644 --- a/src/compiler/allocation-builder-inl.h +++ b/src/compiler/allocation-builder-inl.h @@ -14,11 +14,9 @@ namespace v8 { namespace internal { namespace compiler { -void AllocationBuilder::AllocateContext(int variadic_part_length, - Handle map) { - DCHECK( - IsInRange(map->instance_type(), FIRST_CONTEXT_TYPE, LAST_CONTEXT_TYPE)); - DCHECK_NE(NATIVE_CONTEXT_TYPE, map->instance_type()); +void AllocationBuilder::AllocateContext(int variadic_part_length, MapRef map) { + DCHECK(IsInRange(map.instance_type(), FIRST_CONTEXT_TYPE, LAST_CONTEXT_TYPE)); + DCHECK_NE(NATIVE_CONTEXT_TYPE, map.instance_type()); int size = Context::SizeFor(variadic_part_length); Allocate(size, AllocationType::kYoung, Type::OtherInternal()); Store(AccessBuilder::ForMap(), map); @@ -29,11 +27,11 @@ void AllocationBuilder::AllocateContext(int variadic_part_length, } // Compound allocation of a FixedArray. -void AllocationBuilder::AllocateArray(int length, Handle map, +void AllocationBuilder::AllocateArray(int length, MapRef map, AllocationType allocation) { - DCHECK(map->instance_type() == FIXED_ARRAY_TYPE || - map->instance_type() == FIXED_DOUBLE_ARRAY_TYPE); - int size = (map->instance_type() == FIXED_ARRAY_TYPE) + DCHECK(map.instance_type() == FIXED_ARRAY_TYPE || + map.instance_type() == FIXED_DOUBLE_ARRAY_TYPE); + int size = (map.instance_type() == FIXED_ARRAY_TYPE) ? FixedArray::SizeFor(length) : FixedDoubleArray::SizeFor(length); Allocate(size, allocation, Type::OtherInternal()); diff --git a/src/compiler/allocation-builder.h b/src/compiler/allocation-builder.h index d92e0f769b..040dd01405 100644 --- a/src/compiler/allocation-builder.h +++ b/src/compiler/allocation-builder.h @@ -49,16 +49,12 @@ class AllocationBuilder final { } // Compound allocation of a context. - inline void AllocateContext(int variadic_part_length, Handle map); + inline void AllocateContext(int variadic_part_length, MapRef map); // Compound allocation of a FixedArray. - inline void AllocateArray(int length, Handle map, + inline void AllocateArray(int length, MapRef map, AllocationType allocation = AllocationType::kYoung); - // Compound store of a constant into a field. - void Store(const FieldAccess& access, Handle value) { - Store(access, jsgraph()->Constant(value)); - } // Compound store of a constant into a field. void Store(const FieldAccess& access, const ObjectRef& value) { Store(access, jsgraph()->Constant(value)); diff --git a/src/compiler/js-create-lowering.cc b/src/compiler/js-create-lowering.cc index 6c28fafd4d..be1087871c 100644 --- a/src/compiler/js-create-lowering.cc +++ b/src/compiler/js-create-lowering.cc @@ -406,7 +406,7 @@ Reduction JSCreateLowering::ReduceJSCreateGeneratorObject(Node* node) { int size = parameter_count_no_receiver + shared.GetBytecodeArray().register_count(); AllocationBuilder ab(jsgraph(), effect, control); - ab.AllocateArray(size, factory()->fixed_array_map()); + ab.AllocateArray(size, MapRef(broker(), factory()->fixed_array_map())); for (int i = 0; i < size; ++i) { ab.Store(AccessBuilder::ForFixedArraySlot(i), jsgraph()->UndefinedConstant()); @@ -762,7 +762,8 @@ Reduction JSCreateLowering::ReduceJSCreateAsyncFunctionObject(Node* node) { // Create the register file. AllocationBuilder ab(jsgraph(), effect, control); - ab.AllocateArray(register_count, factory()->fixed_array_map()); + ab.AllocateArray(register_count, + MapRef(broker(), factory()->fixed_array_map())); for (int i = 0; i < register_count; ++i) { ab.Store(AccessBuilder::ForFixedArraySlot(i), jsgraph()->UndefinedConstant()); @@ -873,7 +874,7 @@ Reduction JSCreateLowering::ReduceJSCreateBoundFunction(Node* node) { Node* bound_arguments = jsgraph()->EmptyFixedArrayConstant(); if (arity > 0) { AllocationBuilder a(jsgraph(), effect, control); - a.AllocateArray(arity, factory()->fixed_array_map()); + a.AllocateArray(arity, MapRef(broker(), factory()->fixed_array_map())); for (int i = 0; i < arity; ++i) { a.Store(AccessBuilder::ForFixedArraySlot(i), NodeProperties::GetValueInput(node, 2 + i)); @@ -1019,7 +1020,7 @@ Reduction JSCreateLowering::ReduceJSCreateKeyValueArray(Node* node) { Node* length = jsgraph()->Constant(2); AllocationBuilder aa(jsgraph(), effect, graph()->start()); - aa.AllocateArray(2, factory()->fixed_array_map()); + aa.AllocateArray(2, MapRef(broker(), factory()->fixed_array_map())); aa.Store(AccessBuilder::ForFixedArrayElement(PACKED_ELEMENTS), jsgraph()->ZeroConstant(), key); aa.Store(AccessBuilder::ForFixedArrayElement(PACKED_ELEMENTS), @@ -1206,7 +1207,7 @@ Reduction JSCreateLowering::ReduceJSCreateFunctionContext(Node* node) { default: UNREACHABLE(); } - a.AllocateContext(context_length, map); + a.AllocateContext(context_length, MapRef(broker(), map)); a.Store(AccessBuilder::ForContextSlot(Context::SCOPE_INFO_INDEX), scope_info); a.Store(AccessBuilder::ForContextSlot(Context::PREVIOUS_INDEX), context); @@ -1234,7 +1235,8 @@ Reduction JSCreateLowering::ReduceJSCreateWithContext(Node* node) { AllocationBuilder a(jsgraph(), effect, control); STATIC_ASSERT(Context::MIN_CONTEXT_SLOTS == 4); // Ensure fully covered. - a.AllocateContext(Context::MIN_CONTEXT_SLOTS, factory()->with_context_map()); + a.AllocateContext(Context::MIN_CONTEXT_SLOTS, + MapRef(broker(), factory()->with_context_map())); a.Store(AccessBuilder::ForContextSlot(Context::SCOPE_INFO_INDEX), scope_info); a.Store(AccessBuilder::ForContextSlot(Context::PREVIOUS_INDEX), context); a.Store(AccessBuilder::ForContextSlot(Context::EXTENSION_INDEX), extension); @@ -1257,7 +1259,7 @@ Reduction JSCreateLowering::ReduceJSCreateCatchContext(Node* node) { AllocationBuilder a(jsgraph(), effect, control); STATIC_ASSERT(Context::MIN_CONTEXT_SLOTS == 4); // Ensure fully covered. a.AllocateContext(Context::MIN_CONTEXT_SLOTS + 1, - factory()->catch_context_map()); + MapRef(broker(), factory()->catch_context_map())); a.Store(AccessBuilder::ForContextSlot(Context::SCOPE_INFO_INDEX), scope_info); a.Store(AccessBuilder::ForContextSlot(Context::PREVIOUS_INDEX), context); a.Store(AccessBuilder::ForContextSlot(Context::EXTENSION_INDEX), extension); @@ -1285,7 +1287,8 @@ Reduction JSCreateLowering::ReduceJSCreateBlockContext(Node* node) { AllocationBuilder a(jsgraph(), effect, control); STATIC_ASSERT(Context::MIN_CONTEXT_SLOTS == 4); // Ensure fully covered. - a.AllocateContext(context_length, factory()->block_context_map()); + a.AllocateContext(context_length, + MapRef(broker(), factory()->block_context_map())); a.Store(AccessBuilder::ForContextSlot(Context::SCOPE_INFO_INDEX), scope_info); a.Store(AccessBuilder::ForContextSlot(Context::PREVIOUS_INDEX), context); @@ -1415,7 +1418,8 @@ Node* JSCreateLowering::AllocateArguments(Node* effect, Node* control, // Actually allocate the backing store. AllocationBuilder a(jsgraph(), effect, control); - a.AllocateArray(argument_count, factory()->fixed_array_map()); + a.AllocateArray(argument_count, + MapRef(broker(), factory()->fixed_array_map())); for (int i = 0; i < argument_count; ++i, ++parameters_it) { DCHECK_NOT_NULL((*parameters_it).node); a.Store(AccessBuilder::ForFixedArrayElement(), jsgraph()->Constant(i), @@ -1446,7 +1450,7 @@ Node* JSCreateLowering::AllocateRestArguments(Node* effect, Node* control, // Actually allocate the backing store. AllocationBuilder a(jsgraph(), effect, control); - a.AllocateArray(num_elements, factory()->fixed_array_map()); + a.AllocateArray(num_elements, MapRef(broker(), factory()->fixed_array_map())); for (int i = 0; i < num_elements; ++i, ++parameters_it) { DCHECK_NOT_NULL((*parameters_it).node); a.Store(AccessBuilder::ForFixedArrayElement(), jsgraph()->Constant(i), @@ -1485,7 +1489,8 @@ Node* JSCreateLowering::AllocateAliasedArguments( // another indirection away and then linked into the parameter map below, // whereas mapped argument values are replaced with a hole instead. AllocationBuilder aa(jsgraph(), effect, control); - aa.AllocateArray(argument_count, factory()->fixed_array_map()); + aa.AllocateArray(argument_count, + MapRef(broker(), factory()->fixed_array_map())); for (int i = 0; i < mapped_count; ++i, ++parameters_it) { aa.Store(AccessBuilder::ForFixedArrayElement(), jsgraph()->Constant(i), jsgraph()->TheHoleConstant()); @@ -1499,7 +1504,8 @@ Node* JSCreateLowering::AllocateAliasedArguments( // Actually allocate the backing store. AllocationBuilder a(jsgraph(), arguments, control); - a.AllocateArray(mapped_count + 2, factory()->sloppy_arguments_elements_map()); + a.AllocateArray(mapped_count + 2, + MapRef(broker(), factory()->sloppy_arguments_elements_map())); a.Store(AccessBuilder::ForFixedArrayElement(), jsgraph()->Constant(0), context); a.Store(AccessBuilder::ForFixedArrayElement(), jsgraph()->Constant(1), @@ -1544,7 +1550,8 @@ Node* JSCreateLowering::AllocateAliasedArguments( // Actually allocate the backing store. AllocationBuilder a(jsgraph(), arguments, control); - a.AllocateArray(mapped_count + 2, factory()->sloppy_arguments_elements_map()); + a.AllocateArray(mapped_count + 2, + MapRef(broker(), factory()->sloppy_arguments_elements_map())); a.Store(AccessBuilder::ForFixedArrayElement(), jsgraph()->Constant(0), context); a.Store(AccessBuilder::ForFixedArrayElement(), jsgraph()->Constant(1), @@ -1579,7 +1586,7 @@ Node* JSCreateLowering::AllocateElements(Node* effect, Node* control, // Actually allocate the backing store. AllocationBuilder a(jsgraph(), effect, control); - a.AllocateArray(capacity, elements_map, allocation); + a.AllocateArray(capacity, MapRef(broker(), elements_map), allocation); for (int i = 0; i < capacity; ++i) { Node* index = jsgraph()->Constant(i); a.Store(access, index, value); @@ -1604,7 +1611,7 @@ Node* JSCreateLowering::AllocateElements(Node* effect, Node* control, // Actually allocate the backing store. AllocationBuilder a(jsgraph(), effect, control); - a.AllocateArray(capacity, elements_map, allocation); + a.AllocateArray(capacity, MapRef(broker(), elements_map), allocation); for (int i = 0; i < capacity; ++i) { Node* index = jsgraph()->Constant(i); a.Store(access, index, values[i]); @@ -1670,7 +1677,8 @@ Node* JSCreateLowering::AllocateFastLiteral(Node* effect, Node* control, // Allocate a mutable HeapNumber box and store the value into it. AllocationBuilder builder(jsgraph(), effect, control); builder.Allocate(HeapNumber::kSize, allocation); - builder.Store(AccessBuilder::ForMap(), factory()->heap_number_map()); + builder.Store(AccessBuilder::ForMap(), + MapRef(broker(), factory()->heap_number_map())); builder.Store(AccessBuilder::ForHeapNumberValue(), jsgraph()->Constant(number)); value = effect = builder.Finish(); @@ -1763,7 +1771,7 @@ Node* JSCreateLowering::AllocateFastLiteralElements(Node* effect, Node* control, // Allocate the backing store array and store the elements. AllocationBuilder builder(jsgraph(), effect, control); - builder.AllocateArray(elements_length, elements_map.object(), allocation); + builder.AllocateArray(elements_length, elements_map, allocation); ElementAccess const access = (elements_map.instance_type() == FIXED_DOUBLE_ARRAY_TYPE) ? AccessBuilder::ForFixedDoubleArrayElement() diff --git a/src/compiler/js-graph.cc b/src/compiler/js-graph.cc index 43a4beadee..beed7820b4 100644 --- a/src/compiler/js-graph.cc +++ b/src/compiler/js-graph.cc @@ -46,26 +46,6 @@ Node* JSGraph::CEntryStubConstant(int result_size, SaveFPRegsMode save_doubles, argv_mode, builtin_exit_frame)); } -Node* JSGraph::Constant(Handle value) { - // Dereference the handle to determine if a number constant or other - // canonicalized node can be used. - if (value->IsNumber()) { - return Constant(value->Number()); - } else if (value->IsUndefined(isolate())) { - return UndefinedConstant(); - } else if (value->IsTrue(isolate())) { - return TrueConstant(); - } else if (value->IsFalse(isolate())) { - return FalseConstant(); - } else if (value->IsNull(isolate())) { - return NullConstant(); - } else if (value->IsTheHole(isolate())) { - return TheHoleConstant(); - } else { - return HeapConstant(Handle::cast(value)); - } -} - Node* JSGraph::Constant(const ObjectRef& ref) { if (ref.IsSmi()) return Constant(ref.AsSmi()); OddballType oddball_type = diff --git a/src/compiler/js-graph.h b/src/compiler/js-graph.h index ec36c26034..83c81b1010 100644 --- a/src/compiler/js-graph.h +++ b/src/compiler/js-graph.h @@ -46,16 +46,12 @@ class V8_EXPORT_PRIVATE JSGraph : public MachineGraph { // Used for stubs and runtime functions with no context. (alias: SMI zero) Node* NoContextConstant() { return ZeroConstant(); } - // Creates a HeapConstant node, possibly canonicalized, and may access the - // heap to inspect the object. + // Creates a HeapConstant node, possibly canonicalized. Node* HeapConstant(Handle value); // Creates a Constant node of the appropriate type for the given object. - // Accesses the heap to inspect the object and determine whether one of the + // Inspect the (serialized) object and determine whether one of the // canonicalized globals or a number constant should be returned. - Node* Constant(Handle value); - - // Like above, but doesn't access the heap directly. Node* Constant(const ObjectRef& value); // Creates a NumberConstant node, usually canonicalized. diff --git a/src/compiler/js-native-context-specialization.cc b/src/compiler/js-native-context-specialization.cc index 8c2b7f366a..8969115fad 100644 --- a/src/compiler/js-native-context-specialization.cc +++ b/src/compiler/js-native-context-specialization.cc @@ -2268,7 +2268,8 @@ JSNativeContextSpecialization::BuildPropertyStore( AllocationBuilder a(jsgraph(), effect, control); a.Allocate(HeapNumber::kSize, AllocationType::kYoung, Type::OtherInternal()); - a.Store(AccessBuilder::ForMap(), factory()->heap_number_map()); + a.Store(AccessBuilder::ForMap(), + MapRef(broker(), factory()->heap_number_map())); FieldAccess value_field_access = AccessBuilder::ForHeapNumberValue(); value_field_access.const_field_info = field_access.const_field_info; diff --git a/test/cctest/compiler/test-js-constant-cache.cc b/test/cctest/compiler/test-js-constant-cache.cc index aef10b472d..cc2eddd1a0 100644 --- a/test/cctest/compiler/test-js-constant-cache.cc +++ b/test/cctest/compiler/test-js-constant-cache.cc @@ -4,6 +4,7 @@ #include "src/codegen/assembler.h" #include "src/compiler/js-graph.h" +#include "src/compiler/js-heap-broker.h" #include "src/compiler/node-properties.h" #include "src/heap/factory-inl.h" #include "test/cctest/cctest.h" @@ -35,7 +36,9 @@ class JSConstantCacheTester : public HandleAndZoneScope, JSConstantCacheTester() : JSCacheTesterHelper(main_zone()), JSGraph(main_isolate(), &main_graph_, &main_common_, &main_javascript_, - nullptr, &main_machine_) { + nullptr, &main_machine_), + canonical_(main_isolate()), + broker_(main_isolate(), main_zone(), false) { main_graph_.SetStart(main_graph_.NewNode(common()->Start(0))); main_graph_.SetEnd( main_graph_.NewNode(common()->End(1), main_graph_.start())); @@ -47,6 +50,11 @@ class JSConstantCacheTester : public HandleAndZoneScope, } Factory* factory() { return main_isolate()->factory(); } + JSHeapBroker* broker() { return &broker_; } + + private: + CanonicalHandleScope canonical_; + JSHeapBroker broker_; }; @@ -182,8 +190,8 @@ TEST(HeapNumbers) { Handle num = T.factory()->NewNumber(value); Handle heap = T.factory()->NewHeapNumber(value); Node* node1 = T.Constant(value); - Node* node2 = T.Constant(num); - Node* node3 = T.Constant(heap); + Node* node2 = T.Constant(ObjectRef(T.broker(), num)); + Node* node3 = T.Constant(ObjectRef(T.broker(), heap)); CHECK_EQ(node1, node2); CHECK_EQ(node1, node3); } @@ -193,12 +201,18 @@ TEST(HeapNumbers) { TEST(OddballHandle) { JSConstantCacheTester T; - CHECK_EQ(T.UndefinedConstant(), T.Constant(T.factory()->undefined_value())); - CHECK_EQ(T.TheHoleConstant(), T.Constant(T.factory()->the_hole_value())); - CHECK_EQ(T.TrueConstant(), T.Constant(T.factory()->true_value())); - CHECK_EQ(T.FalseConstant(), T.Constant(T.factory()->false_value())); - CHECK_EQ(T.NullConstant(), T.Constant(T.factory()->null_value())); - CHECK_EQ(T.NaNConstant(), T.Constant(T.factory()->nan_value())); + CHECK_EQ(T.UndefinedConstant(), + T.Constant(ObjectRef(T.broker(), T.factory()->undefined_value()))); + CHECK_EQ(T.TheHoleConstant(), + T.Constant(ObjectRef(T.broker(), T.factory()->the_hole_value()))); + CHECK_EQ(T.TrueConstant(), + T.Constant(ObjectRef(T.broker(), T.factory()->true_value()))); + CHECK_EQ(T.FalseConstant(), + T.Constant(ObjectRef(T.broker(), T.factory()->false_value()))); + CHECK_EQ(T.NullConstant(), + T.Constant(ObjectRef(T.broker(), T.factory()->null_value()))); + CHECK_EQ(T.NaNConstant(), + T.Constant(ObjectRef(T.broker(), T.factory()->nan_value()))); } diff --git a/test/cctest/compiler/test-js-context-specialization.cc b/test/cctest/compiler/test-js-context-specialization.cc index e6703dbbbe..1b136873b5 100644 --- a/test/cctest/compiler/test-js-context-specialization.cc +++ b/test/cctest/compiler/test-js-context-specialization.cc @@ -52,6 +52,8 @@ class ContextSpecializationTester : public HandleAndZoneScope { void CheckContextInputAndDepthChanges(Node* node, Node* expected_new_context, size_t expected_new_depth); + JSHeapBroker* broker() { return &js_heap_broker_; } + private: TickCounter tick_counter_; CanonicalHandleScope canonical_; @@ -126,8 +128,9 @@ TEST(ReduceJSLoadContext0) { const int slot = Context::NATIVE_CONTEXT_INDEX; native->set(slot, *expected); - Node* const_context = t.jsgraph()->Constant(native); - Node* deep_const_context = t.jsgraph()->Constant(subcontext2); + Node* const_context = t.jsgraph()->Constant(ObjectRef(t.broker(), native)); + Node* deep_const_context = + t.jsgraph()->Constant(ObjectRef(t.broker(), subcontext2)); Node* param_context = t.graph()->NewNode(t.common()->Parameter(0), start); { @@ -269,7 +272,8 @@ TEST(ReduceJSLoadContext2) { context_object0->set(slot_index, *slot_value0); context_object1->set(slot_index, *slot_value1); - Node* context0 = t.jsgraph()->Constant(context_object1); + Node* context0 = + t.jsgraph()->Constant(ObjectRef(t.broker(), context_object1)); Node* context1 = t.graph()->NewNode(create_function_context, context0, start, start); Node* context2 = @@ -423,8 +427,9 @@ TEST(ReduceJSStoreContext0) { const int slot = Context::NATIVE_CONTEXT_INDEX; native->set(slot, *expected); - Node* const_context = t.jsgraph()->Constant(native); - Node* deep_const_context = t.jsgraph()->Constant(subcontext2); + Node* const_context = t.jsgraph()->Constant(ObjectRef(t.broker(), native)); + Node* deep_const_context = + t.jsgraph()->Constant(ObjectRef(t.broker(), subcontext2)); Node* param_context = t.graph()->NewNode(t.common()->Parameter(0), start); { @@ -531,7 +536,8 @@ TEST(ReduceJSStoreContext2) { context_object0->set(slot_index, *slot_value0); context_object1->set(slot_index, *slot_value1); - Node* context0 = t.jsgraph()->Constant(context_object1); + Node* context0 = + t.jsgraph()->Constant(ObjectRef(t.broker(), context_object1)); Node* context1 = t.graph()->NewNode(create_function_context, context0, start, start); Node* context2 =