[vm/compiler] Fix ARM7 regression after 1827fcbf68.

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 <kustermann@google.com>
Commit-Queue: Tess Strickland <sstrickl@google.com>

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

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,

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

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