From 49f1450b3f967d0b0bef11b4c8eb28593c4004c8 Mon Sep 17 00:00:00 2001 From: Victor Gomes Date: Fri, 3 Feb 2023 11:17:18 +0100 Subject: [PATCH] Reland "[maglev] Check for strings in polymorphic loads" This is a reland of commit 7f4a04671a33d119c460e6e6eac14bf6fa17c8a6 - Add heap number check. - Use object_map in range instance check. Original change's description: > [maglev] Check for strings in polymorphic loads > > Bug: v8:7700 > Change-Id: Id3d523446f5061a78a46d1c52cf8f8339566356d > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4212402 > Reviewed-by: Leszek Swirski > Commit-Queue: Leszek Swirski > Auto-Submit: Victor Gomes > Cr-Commit-Position: refs/heads/main@{#85626} Bug: v8:7700 Change-Id: I72cfe2e2bf19141dffbb8df5c34600eca4d70594 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4218508 Commit-Queue: Victor Gomes Auto-Submit: Victor Gomes Reviewed-by: Leszek Swirski Cr-Commit-Position: refs/heads/main@{#85642} --- src/maglev/arm64/maglev-assembler-arm64-inl.h | 14 +++++++ src/maglev/maglev-assembler.h | 6 +++ src/maglev/maglev-graph-builder.cc | 17 --------- src/maglev/maglev-ir.cc | 37 ++++++++++--------- src/maglev/maglev-ir.h | 14 +++++++ src/maglev/x64/maglev-assembler-x64-inl.h | 11 ++++++ .../mjsunit/maglev/polymorphic-load-number.js | 18 +++++++++ 7 files changed, 83 insertions(+), 34 deletions(-) create mode 100644 test/mjsunit/maglev/polymorphic-load-number.js diff --git a/src/maglev/arm64/maglev-assembler-arm64-inl.h b/src/maglev/arm64/maglev-assembler-arm64-inl.h index 5c8f006d51..49ade36a2d 100644 --- a/src/maglev/arm64/maglev-assembler-arm64-inl.h +++ b/src/maglev/arm64/maglev-assembler-arm64-inl.h @@ -519,6 +519,20 @@ inline void MaglevAssembler::CompareObjectTypeRange(Register heap_object, CompareInstanceTypeRange(scratch, scratch, lower_limit, higher_limit); } +inline void MaglevAssembler::CompareInstanceTypeRange( + Register map, InstanceType lower_limit, InstanceType higher_limit) { + ScratchRegisterScope temps(this); + Register scratch = temps.Acquire(); + CompareInstanceTypeRange(map, scratch, lower_limit, higher_limit); +} + +inline void MaglevAssembler::CompareInstanceTypeRange( + Register map, Register instance_type_out, InstanceType lower_limit, + InstanceType higher_limit) { + MacroAssembler::CompareInstanceTypeRange(map, instance_type_out, lower_limit, + higher_limit); +} + inline void MaglevAssembler::CompareTagged(Register reg, Handle obj) { ScratchRegisterScope temps(this); diff --git a/src/maglev/maglev-assembler.h b/src/maglev/maglev-assembler.h index c22fa9fda5..35c8fc4b8a 100644 --- a/src/maglev/maglev-assembler.h +++ b/src/maglev/maglev-assembler.h @@ -199,6 +199,12 @@ class MaglevAssembler : public MacroAssembler { InstanceType lower_limit, InstanceType higher_limit); + inline void CompareInstanceTypeRange(Register map, InstanceType lower_limit, + InstanceType higher_limit); + inline void CompareInstanceTypeRange(Register map, Register instance_type_out, + InstanceType lower_limit, + InstanceType higher_limit); + inline void CompareTagged(Register reg, Handle obj); inline void CompareInt32(Register reg, int32_t imm); diff --git a/src/maglev/maglev-graph-builder.cc b/src/maglev/maglev-graph-builder.cc index fe35cd0e79..134a80bd73 100644 --- a/src/maglev/maglev-graph-builder.cc +++ b/src/maglev/maglev-graph-builder.cc @@ -2005,23 +2005,6 @@ bool MaglevGraphBuilder::TryBuildPropertyAccess( } } -namespace { -bool HasOnlyStringMaps(base::Vector maps) { - for (compiler::MapRef map : maps) { - if (!map.IsStringMap()) return false; - } - return true; -} - -bool HasOnlyNumberMaps(base::Vector maps) { - for (compiler::MapRef map : maps) { - if (map.instance_type() != HEAP_NUMBER_TYPE) return false; - } - return true; -} - -} // namespace - bool MaglevGraphBuilder::TryBuildNamedAccess( ValueNode* receiver, ValueNode* lookup_start_object, compiler::NamedAccessFeedback const& feedback, diff --git a/src/maglev/maglev-ir.cc b/src/maglev/maglev-ir.cc index 1b552f8e37..9b852f13c7 100644 --- a/src/maglev/maglev-ir.cc +++ b/src/maglev/maglev-ir.cc @@ -988,26 +988,29 @@ void EmitPolymorphicAccesses(MaglevAssembler* masm, NodeT* node, for (const PolymorphicAccessInfo& access_info : node->access_infos()) { Label next; Label map_found; - bool has_heap_number_map = false; + auto& maps = access_info.maps(); - for (auto it = access_info.maps().begin(); it != access_info.maps().end(); - ++it) { - if (it->IsHeapNumberMap()) { - has_heap_number_map = true; - } - __ CompareTagged(object_map, it->object()); - if (it == access_info.maps().end() - 1) { - __ JumpIf(kNotEqual, &next); - // Fallthrough... to map_found. - } else { - __ JumpIf(kEqual, &map_found); - } - } - - // Bind number case here if one of the maps is HeapNumber. - if (has_heap_number_map) { + if (HasOnlyNumberMaps(base::VectorOf(maps))) { + __ CompareRoot(object_map, RootIndex::kHeapNumberMap); + __ JumpIf(kNotEqual, &next); + // Fallthrough... to map_found. DCHECK(!is_number.is_bound()); __ bind(&is_number); + } else if (HasOnlyStringMaps(base::VectorOf(maps))) { + __ CompareInstanceTypeRange(object_map, FIRST_STRING_TYPE, + LAST_STRING_TYPE); + __ JumpIf(kUnsignedGreaterThan, &next); + // Fallthrough... to map_found. + } else { + for (auto it = maps.begin(); it != maps.end(); ++it) { + __ CompareTagged(object_map, it->object()); + if (it == maps.end() - 1) { + __ JumpIf(kNotEqual, &next); + // Fallthrough... to map_found. + } else { + __ JumpIf(kEqual, &map_found); + } + } } __ bind(&map_found); diff --git a/src/maglev/maglev-ir.h b/src/maglev/maglev-ir.h index d02b9ee4fe..5781ca2d16 100644 --- a/src/maglev/maglev-ir.h +++ b/src/maglev/maglev-ir.h @@ -463,6 +463,20 @@ inline std::ostream& operator<<(std::ostream& os, return os; } +inline bool HasOnlyStringMaps(base::Vector maps) { + for (compiler::MapRef map : maps) { + if (!map.IsStringMap()) return false; + } + return true; +} + +inline bool HasOnlyNumberMaps(base::Vector maps) { + for (compiler::MapRef map : maps) { + if (map.instance_type() != HEAP_NUMBER_TYPE) return false; + } + return true; +} + #define DEF_FORWARD_DECLARATION(type, ...) class type; NODE_BASE_LIST(DEF_FORWARD_DECLARATION) #undef DEF_FORWARD_DECLARATION diff --git a/src/maglev/x64/maglev-assembler-x64-inl.h b/src/maglev/x64/maglev-assembler-x64-inl.h index b4104040d9..390ea8ab96 100644 --- a/src/maglev/x64/maglev-assembler-x64-inl.h +++ b/src/maglev/x64/maglev-assembler-x64-inl.h @@ -405,6 +405,17 @@ inline void MaglevAssembler::CompareObjectTypeRange(Register heap_object, higher_limit); } +inline void MaglevAssembler::CompareInstanceTypeRange( + Register map, InstanceType lower_limit, InstanceType higher_limit) { + CompareInstanceTypeRange(map, kScratchRegister, lower_limit, higher_limit); +} + +inline void MaglevAssembler::CompareInstanceTypeRange( + Register map, Register instance_type_out, InstanceType lower_limit, + InstanceType higher_limit) { + CmpInstanceTypeRange(map, instance_type_out, lower_limit, higher_limit); +} + inline void MaglevAssembler::CompareTagged(Register reg, Handle obj) { Cmp(reg, obj); diff --git a/test/mjsunit/maglev/polymorphic-load-number.js b/test/mjsunit/maglev/polymorphic-load-number.js new file mode 100644 index 0000000000..cfda707d7c --- /dev/null +++ b/test/mjsunit/maglev/polymorphic-load-number.js @@ -0,0 +1,18 @@ +// Copyright 2023 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 --maglev + + +function foo(o) { + return o.length; +} + +%PrepareFunctionForOptimization(foo); +assertEquals(6, foo("string")); +assertEquals(undefined, foo(4.2)); + +%OptimizeMaglevOnNextCall(foo); +assertEquals(6, foo("string")); +assertEquals(undefined, foo(4.2));