From 3feab3655c93b8cda3ea09e173fa2a21eec35f39 Mon Sep 17 00:00:00 2001 From: Tess Strickland Date: Tue, 16 Jan 2024 12:13:41 +0000 Subject: [PATCH] [vm/compiler] Fix ARM7 regression after 1827fcbf68bf. The refactoring in https://dart-review.googlesource.com/c/sdk/+/345002 erroneously assumed that if the caller to Assembler::AddressCanHoldConstantIndex didn't pass an out parameter to detect whether a base register was needed, that that case shouldn't be considered. While it's true that LoadIndexedInstr::MakeLocationSummary originally ignored the value stored in the out parameter, it did that because it can use TMP to store the array base, unlike the code for StoreIndexedInstr which must have a temporary register allocated during register allocation. Thus, fix Assembler::AddressCanHoldConstantIndex to always require the out parameter, and add a comment in LoadIndexedInstr as to why we ignore the out parameter in this case instead of allocating an additional temporary register. TEST=vm/cc/Assembler_Regress54621 Fixes: https://github.com/dart-lang/sdk/issues/54621 Change-Id: I0af557565faf657a87641457884334446e9b7cc5 Cq-Include-Trybots: luci.dart.try:vm-ffi-qemu-linux-release-arm-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/346201 Reviewed-by: Martin Kustermann Commit-Queue: Tess Strickland --- .../vm/compiler/assembler/assembler_arm.cc | 13 +++------ runtime/vm/compiler/assembler/assembler_arm.h | 3 +++ .../vm/compiler/assembler/assembler_test.cc | 27 +++++++++++++++++++ runtime/vm/compiler/backend/il_arm.cc | 12 ++++++--- 4 files changed, 42 insertions(+), 13 deletions(-) diff --git a/runtime/vm/compiler/assembler/assembler_arm.cc b/runtime/vm/compiler/assembler/assembler_arm.cc index 1e9f856ca21..d5bdb539584 100644 --- a/runtime/vm/compiler/assembler/assembler_arm.cc +++ b/runtime/vm/compiler/assembler/assembler_arm.cc @@ -3738,6 +3738,7 @@ bool Assembler::AddressCanHoldConstantIndex(const Object& constant, intptr_t cid, intptr_t index_scale, bool* needs_base) { + ASSERT(needs_base != nullptr); if ((cid == kTypedDataInt32x4ArrayCid) || (cid == kTypedDataFloat32x4ArrayCid) || (cid == kTypedDataFloat64x2ArrayCid)) { @@ -3751,18 +3752,12 @@ bool Assembler::AddressCanHoldConstantIndex(const Object& constant, (is_external ? 0 : (target::Instance::DataOffsetFor(cid) - kHeapObjectTag)); const int64_t offset = index * index_scale + offset_base; - if (!Utils::MagnitudeIsUint(12, offset)) { - return false; - } + ASSERT(Utils::IsInt(32, offset)); if (Address::CanHoldImmediateOffset(is_load, cid, offset)) { - if (needs_base != nullptr) { - *needs_base = false; - } + *needs_base = false; return true; } - - if (needs_base != nullptr && - Address::CanHoldImmediateOffset(is_load, cid, offset - offset_base)) { + if (Address::CanHoldImmediateOffset(is_load, cid, offset - offset_base)) { *needs_base = true; return true; } diff --git a/runtime/vm/compiler/assembler/assembler_arm.h b/runtime/vm/compiler/assembler/assembler_arm.h index ce276948a5f..4f574945520 100644 --- a/runtime/vm/compiler/assembler/assembler_arm.h +++ b/runtime/vm/compiler/assembler/assembler_arm.h @@ -1494,6 +1494,9 @@ class Assembler : public AssemblerBase { // nearby the load of the table address. void LoadAllocationTracingStateAddress(Register dest, intptr_t cid); + // If true is returned, then the out parameter [need_base] signifies whether + // a register is needed for storing the array base (which should be passed + // as the [temp] parameter to ElementAddressForIntIndex). static bool AddressCanHoldConstantIndex(const Object& constant, bool is_load, bool is_external, diff --git a/runtime/vm/compiler/assembler/assembler_test.cc b/runtime/vm/compiler/assembler/assembler_test.cc index cac4cfc80af..754c9e42e10 100644 --- a/runtime/vm/compiler/assembler/assembler_test.cc +++ b/runtime/vm/compiler/assembler/assembler_test.cc @@ -216,4 +216,31 @@ LOAD_FROM_BOX_TEST(Int64, int64_t, 0xFFFFFFFFFFFFFFFF, true) LOAD_FROM_BOX_TEST(Int64, int64_t, 0xFFFFFFFFFFFFFFFF, false) #endif +#if defined(TARGET_ARCH_ARM) +ISOLATE_UNIT_TEST_CASE(Assembler_Regress54621) { + auto zone = thread->zone(); + const classid_t cid = kTypedDataInt32ArrayCid; + const intptr_t index_scale = 1; + const intptr_t kMaxAllowedOffsetForExternal = (2 << 11) - 1; + auto& smi = Smi::Handle(zone, Smi::New(kMaxAllowedOffsetForExternal)); + bool needs_base; + EXPECT(compiler::Assembler::AddressCanHoldConstantIndex( + smi, /*is_load=*/true, + /*is_external=*/true, cid, index_scale, &needs_base)); + EXPECT(!needs_base); + EXPECT(compiler::Assembler::AddressCanHoldConstantIndex( + smi, /*is_load=*/true, + /*is_external=*/false, cid, index_scale, &needs_base)); + EXPECT(needs_base); + // Double-checking we're on a boundary of what's allowed. + smi = Smi::New(kMaxAllowedOffsetForExternal + 1); + EXPECT(!compiler::Assembler::AddressCanHoldConstantIndex( + smi, /*is_load=*/true, + /*is_external=*/false, cid, index_scale)); + EXPECT(!compiler::Assembler::AddressCanHoldConstantIndex( + smi, /*is_load=*/true, + /*is_external=*/true, cid, index_scale)); +} +#endif + } // namespace dart diff --git a/runtime/vm/compiler/backend/il_arm.cc b/runtime/vm/compiler/backend/il_arm.cc index b8aaf43c526..28d350e8cb0 100644 --- a/runtime/vm/compiler/backend/il_arm.cc +++ b/runtime/vm/compiler/backend/il_arm.cc @@ -2119,10 +2119,14 @@ LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone, LocationSummary* locs = new (zone) LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall); locs->set_in(0, Location::RequiresRegister()); - const bool can_be_constant = index()->BindsToConstant() && - compiler::Assembler::AddressCanHoldConstantIndex( - index()->BoundConstant(), /*load=*/true, - IsExternal(), class_id(), index_scale()); + bool needs_base; + const bool can_be_constant = + index()->BindsToConstant() && + compiler::Assembler::AddressCanHoldConstantIndex( + index()->BoundConstant(), /*load=*/true, IsExternal(), class_id(), + index_scale(), &needs_base); + // We don't need to check if [needs_base] is true, since we use TMP as the + // temp register in this case and so don't need to allocate a temp register. locs->set_in(1, can_be_constant ? Location::Constant(index()->definition()->AsConstant()) : Location::RequiresRegister());