Revert "[arm] Make the constant pool check deadline smarter"

This reverts commit df4dae77.

Reason for revert: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Arm%20-%20debug/18512/overview

Original change's description:
> [arm] Make the constant pool check deadline smarter
>
> Rather than having periodic constant pool checks that almost always fail
> (because the constant pool deadline isn't close enough, or even because
> there's no constant pool to emit at all), set a single deadline on the
> first constant pool insertion which expires just before the maximum
> distance to the constant pool. Constant pool checks around unconditional
> jumps happen irrespective of this deadline.
>
> In particular, this is made possible by fixing the incorrect assumption
> that the constant pool can be emitted out of order. The new assumption
> (that the emission is in-order) is validated with a CHECK.
>
> Bug: v8:11420
> Change-Id: I061dd0b8c3476ba95ee1acfb3b485d8ba2adda91
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2844665
> Commit-Queue: Leszek Swirski <leszeks@chromium.org>
> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#74141}

Bug: v8:11420
Change-Id: Ib822425749df33fb22a38d317c107a523b382e01
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2846899
Auto-Submit: Sathya Gunasekaran  <gsathya@chromium.org>
Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74144}

src/codegen/arm/assembler-arm.cc

@@ -535,7 +535,7 @@ Assembler::Assembler(const AssemblerOptions& options,
       pending_32_bit_constants_(),
       scratch_register_list_(ip.bit()) {
   reloc_info_writer.Reposition(buffer_start_ + buffer_->size(), pc_);
-  constant_pool_deadline_ = kMaxInt;
+  next_buffer_check_ = 0;
   const_pool_blocked_nesting_ = 0;
   no_const_pool_before_ = 0;
   first_const_pool_32_use_ = -1;
@@ -5246,13 +5246,8 @@ void Assembler::ConstantPoolAddEntry(int position, RelocInfo::Mode rmode,
                     (rmode == RelocInfo::CODE_TARGET && value != 0) ||
                     (RelocInfo::IsEmbeddedObjectMode(rmode) && value != 0);
   DCHECK_LT(pending_32_bit_constants_.size(), kMaxNumPending32Constants);
-  if (first_const_pool_32_use_ < 0) {
-    DCHECK(pending_32_bit_constants_.empty());
-    DCHECK_EQ(constant_pool_deadline_, kMaxInt);
+  if (pending_32_bit_constants_.empty()) {
     first_const_pool_32_use_ = position;
-    constant_pool_deadline_ = position + kCheckPoolDeadline;
-  } else {
-    DCHECK(!pending_32_bit_constants_.empty());
   }
   ConstantPoolEntry entry(position, value, sharing_ok, rmode);
 
@@ -5286,17 +5281,17 @@ void Assembler::ConstantPoolAddEntry(int position, RelocInfo::Mode rmode,
 void Assembler::BlockConstPoolFor(int instructions) {
   int pc_limit = pc_offset() + instructions * kInstrSize;
   if (no_const_pool_before_ < pc_limit) {
+    // Max pool start (if we need a jump and an alignment).
+#ifdef DEBUG
+    int start = pc_limit + kInstrSize + 2 * kPointerSize;
+    DCHECK(pending_32_bit_constants_.empty() ||
+           (start < first_const_pool_32_use_ + kMaxDistToIntPool));
+#endif
     no_const_pool_before_ = pc_limit;
   }
 
-  // If we're due a const pool check before the block finishes, move it to just
-  // after the block.
-  if (constant_pool_deadline_ < no_const_pool_before_) {
-    // Make sure that the new deadline isn't too late (including a jump and the
-    // constant pool marker).
-    DCHECK_LE(no_const_pool_before_,
-              first_const_pool_32_use_ + kMaxDistToIntPool);
-    constant_pool_deadline_ = no_const_pool_before_;
+  if (next_buffer_check_ < no_const_pool_before_) {
+    next_buffer_check_ = no_const_pool_before_;
   }
 }
 
@@ -5312,44 +5307,49 @@ void Assembler::CheckConstPool(bool force_emit, bool require_jump) {
 
   // There is nothing to do if there are no pending constant pool entries.
   if (pending_32_bit_constants_.empty()) {
-    // We should only fall into this case if we're either trying to forcing
-    // emission or opportunistically checking after a jump.
-    DCHECK(force_emit || !require_jump);
+    // Calculate the offset of the next check.
+    next_buffer_check_ = pc_offset() + kCheckPoolInterval;
     return;
   }
 
+  // Check that the code buffer is large enough before emitting the constant
+  // pool (include the jump over the pool and the constant pool marker and
+  // the gap to the relocation information).
+  int jump_instr = require_jump ? kInstrSize : 0;
+  int size_up_to_marker = jump_instr + kInstrSize;
+  int estimated_size_after_marker =
+      pending_32_bit_constants_.size() * kPointerSize;
+  int estimated_size = size_up_to_marker + estimated_size_after_marker;
+
   // We emit a constant pool when:
   //  * requested to do so by parameter force_emit (e.g. after each function).
   //  * the distance from the first instruction accessing the constant pool to
-  //    the first constant pool entry will exceed its limit the next time the
-  //    pool is checked.
+  //    any of the constant pool entries will exceed its limit the next
+  //    time the pool is checked. This is overly restrictive, but we don't emit
+  //    constant pool entries in-order so it's conservatively correct.
   //  * the instruction doesn't require a jump after itself to jump over the
   //    constant pool, and we're getting close to running out of range.
   if (!force_emit) {
-    DCHECK_NE(first_const_pool_32_use_, -1);
-    int dist32 = pc_offset() - first_const_pool_32_use_;
-    if (require_jump) {
-      // We should only be on this path if we've exceeded our deadline.
-      DCHECK_GE(dist32, kCheckPoolDeadline);
-    } else if (dist32 < kCheckPoolDeadline / 2) {
-      return;
+    DCHECK(!pending_32_bit_constants_.empty());
+    bool need_emit = false;
+    int dist32 = pc_offset() + estimated_size - first_const_pool_32_use_;
+    if ((dist32 >= kMaxDistToIntPool - kCheckPoolInterval) ||
+        (!require_jump && (dist32 >= kMaxDistToIntPool / 2))) {
+      need_emit = true;
     }
+    if (!need_emit) return;
   }
 
-  int size_after_marker = pending_32_bit_constants_.size() * kPointerSize;
-
   // Deduplicate constants.
+  int size_after_marker = estimated_size_after_marker;
+
   for (size_t i = 0; i < pending_32_bit_constants_.size(); i++) {
     ConstantPoolEntry& entry = pending_32_bit_constants_[i];
     if (entry.is_merged()) size_after_marker -= kPointerSize;
   }
 
-  // Check that the code buffer is large enough before emitting the constant
-  // pool (include the jump over the pool and the constant pool marker and
-  // the gap to the relocation information).
-  int jump_instr = require_jump ? kInstrSize : 0;
-  int size_up_to_marker = jump_instr + kInstrSize;
   int size = size_up_to_marker + size_after_marker;
+
   int needed_space = size + kGap;
   while (buffer_space() <= needed_space) GrowBuffer();
 
@@ -5373,14 +5373,6 @@ void Assembler::CheckConstPool(bool force_emit, bool require_jump) {
     emit(kConstantPoolMarker |
          EncodeConstantPoolLength(size_after_marker / kPointerSize));
 
-    // The first entry in the constant pool should also be the first
-    CHECK_EQ(first_const_pool_32_use_, pending_32_bit_constants_[0].position());
-    CHECK(!pending_32_bit_constants_[0].is_merged());
-
-    // Make sure we're not emitting the constant too late.
-    CHECK_LE(pc_offset(),
-             first_const_pool_32_use_ + kMaxDistToPcRelativeConstant);
-
     // Emit 32-bit constant pool entries.
     for (size_t i = 0; i < pending_32_bit_constants_.size(); i++) {
       ConstantPoolEntry& entry = pending_32_bit_constants_[i];
@@ -5404,7 +5396,6 @@ void Assembler::CheckConstPool(bool force_emit, bool require_jump) {
         ConstantPoolEntry& merged =
             pending_32_bit_constants_[entry.merged_index()];
         DCHECK(entry.value() == merged.value());
-        DCHECK_LT(merged.position(), entry.position());
         Instr merged_instr = instr_at(merged.position());
         DCHECK(IsLdrPcImmediateOffset(merged_instr));
         delta = GetLdrRegisterImmediateOffset(merged_instr);
@@ -5430,9 +5421,9 @@ void Assembler::CheckConstPool(bool force_emit, bool require_jump) {
     }
   }
 
-  // Since a constant pool was just emitted, we don't need another check until
-  // the next constant pool entry is added.
-  constant_pool_deadline_ = kMaxInt;
+  // Since a constant pool was just emitted, move the check offset forward by
+  // the standard interval.
+  next_buffer_check_ = pc_offset() + kCheckPoolInterval;
 }
 
 PatchingAssembler::PatchingAssembler(const AssemblerOptions& options,

src/codegen/arm/assembler-arm.h

@@ -1149,24 +1149,13 @@ class V8_EXPORT_PRIVATE Assembler : public AssemblerBase {
   static int DecodeShiftImm(Instr instr);
   static Instr PatchShiftImm(Instr instr, int immed);
 
-  // Constants are accessed via pc relative addressing, which can reach −4095 to
-  // 4095 for integer PC-relative loads, and −1020 to 1020 for floating-point
+  // Constants in pools are accessed via pc relative addressing, which can
+  // reach +/-4KB for integer PC-relative loads and +/-1KB for floating-point
   // PC-relative loads, thereby defining a maximum distance between the
-  // instruction and the accessed constant. Additionally, PC-relative loads
-  // start at a delta from the actual load instruction's PC, so we can add this
-  // on to the (positive) distance.
-  static constexpr int kMaxDistToPcRelativeConstant =
-      4095 + Instruction::kPcLoadDelta;
-  // The constant pool needs to be jumped over, and has a marker, so the actual
-  // distance from the instruction and start of the constant pool has to include
-  // space for these two instructions.
-  static constexpr int kMaxDistToIntPool =
-      kMaxDistToPcRelativeConstant - 2 * kInstrSize;
+  // instruction and the accessed constant.
+  static constexpr int kMaxDistToIntPool = 4 * KB;
   // Experimentally derived as sufficient for ~95% of compiles.
   static constexpr int kTypicalNumPending32Constants = 32;
-  // The maximum number of pending constants is reached by a sequence of only
-  // constant loads, which limits it to the number of constant loads that can
-  // fit between the first constant load and the distance to the constant pool.
   static constexpr int kMaxNumPending32Constants =
       kMaxDistToIntPool / kInstrSize;
 
@@ -1178,7 +1167,7 @@ class V8_EXPORT_PRIVATE Assembler : public AssemblerBase {
   void CheckConstPool(bool force_emit, bool require_jump);
 
   V8_INLINE void MaybeCheckConstPool() {
-    if (V8_UNLIKELY(pc_offset() >= constant_pool_deadline_)) {
+    if (V8_UNLIKELY(pc_offset() >= next_buffer_check_)) {
       CheckConstPool(false, true);
     }
   }
@@ -1204,8 +1193,9 @@ class V8_EXPORT_PRIVATE Assembler : public AssemblerBase {
   // number of call to EndBlockConstpool.
   void StartBlockConstPool() {
     if (const_pool_blocked_nesting_++ == 0) {
-      // Prevent constant pool checks happening by resetting the deadline.
-      constant_pool_deadline_ = kMaxInt;
+      // Prevent constant pool checks happening by setting the next check to
+      // the biggest possible offset.
+      next_buffer_check_ = kMaxInt;
     }
   }
 
@@ -1213,14 +1203,19 @@ class V8_EXPORT_PRIVATE Assembler : public AssemblerBase {
   // StartBlockConstPool to have an effect.
   void EndBlockConstPool() {
     if (--const_pool_blocked_nesting_ == 0) {
-      if (first_const_pool_32_use_ >= 0) {
 #ifdef DEBUG
-        // Check the constant pool hasn't been blocked for too long.
-        DCHECK_LE(pc_offset(), first_const_pool_32_use_ + kMaxDistToIntPool);
+      // Max pool start (if we need a jump and an alignment).
+      int start = pc_offset() + kInstrSize + 2 * kPointerSize;
+      // Check the constant pool hasn't been blocked for too long.
+      DCHECK(pending_32_bit_constants_.empty() ||
+             (start < first_const_pool_32_use_ + kMaxDistToIntPool));
 #endif
-        // Reset the constant pool check back to the deadline.
-        constant_pool_deadline_ = first_const_pool_32_use_ + kCheckPoolDeadline;
-      }
+      // Two cases:
+      //  * no_const_pool_before_ >= next_buffer_check_ and the emission is
+      //    still blocked
+      //  * no_const_pool_before_ < next_buffer_check_ and the next emit will
+      //    trigger a check.
+      next_buffer_check_ = no_const_pool_before_;
     }
   }
 
@@ -1275,6 +1270,8 @@ class V8_EXPORT_PRIVATE Assembler : public AssemblerBase {
   // Avoid overflows for displacements etc.
   static const int kMaximalBufferSize = 512 * MB;
 
+  int next_buffer_check_;  // pc offset of next buffer check
+
   // Constant pool generation
   // Pools are emitted in the instruction stream, preferably after unconditional
   // jumps or after returns from functions (in dead code locations).
@@ -1286,16 +1283,11 @@ class V8_EXPORT_PRIVATE Assembler : public AssemblerBase {
   // if so, a relocation info entry is associated to the constant pool entry.
 
   // Repeated checking whether the constant pool should be emitted is rather
-  // expensive. Instead, we check once a deadline is hit; the deadline being
-  // when there is a possibility that MaybeCheckConstPool won't be called before
-  // kMaxDistToIntPoolWithHeader is exceeded. Since MaybeCheckConstPool is
-  // called in CheckBuffer, this means that kGap is an upper bound on this
-  // check. Use 2 * kGap just to give it some slack around BlockConstPoolScopes.
-  static constexpr int kCheckPoolDeadline = kMaxDistToIntPool - 2 * kGap;
-
-  // pc offset of the upcoming constant pool deadline. Equivalent to
-  // first_const_pool_32_use_ + kCheckPoolDeadline.
-  int constant_pool_deadline_;
+  // expensive. By default we only check again once a number of instructions
+  // has been generated. That also means that the sizing of the buffers is not
+  // an exact science, and that we rely on some slop to not overrun buffers.
+  static constexpr int kCheckPoolIntervalInst = 32;
+  static constexpr int kCheckPoolInterval = kCheckPoolIntervalInst * kInstrSize;
 
   // Emission of the constant pool may be blocked in some code sequences.
   int const_pool_blocked_nesting_;  // Block emission if this is not zero.