Skip to content

GVSoC: Various small 64 tile bugs #39

Description

@jpf-h

Hey @Aquaticfuller @DiyouS,

found when running some kernels on the 64 tile variant.

And-gate (and.cpp): input words are uint64_t but bits were set with 1 << bit as an int. Input 31 of a word is INT_MIN and sign-extends into bits 31–63 on the OR, so any gate with ≥ 32 inputs per word can spuriously read "all true"; the last-word mask had the same bad shift plus a wrong result for input counts that are exact multiples of 64. The icache flush-ack gate has cores+1 inputs — 129 at 32 tiles, 257 at 64 — so every many-core config was affected. Fix: 64-bit shifts throughout.

ISS cache_sync (both exec_inorder variants): cleared only in reset(true). The icache's own reset fires the flush-ack fan-out at every core, and a core whose reset hasn't run yet holds an uninitialized cache_sync — true by chance in debug builds — and decrements a zero stall counter: the "Trying to decrease zero stalled counter" fatal that aborted every traced ≥128-core run at reset. Fix: initialize in the constructor.

Requester-side fill (insitu_cache_xbar.cpp): the stream-buffer line fill assumed the home bank answers synchronously; when the bank's refill path returns PENDING, the in-flight fill's response must return to the xbar's fill logic, not to a core with nothing outstanding — otherwise a misrouted response corrupts stall accounting downstream. Fix: per-buffer fill request objects, a registered response handler, demand reads parked on a pending fill, and a corrected write-invalidate scan.

Note: Found by AI, I have not verified the diagnosis and patch below completely and unfortunately don't have time to clean it up now. But I wanted to report before you run into this issue and have to debug it again :-)

I think especially issue/solution 3 should be double-checked.

From c6bf5af12a43f791e40cad0a91ec893cd7df5d47 Mon Sep 17 00:00:00 2001
From: Johannes Pfau <johannes.pfau@h-partners.com>
Date: Tue, 25 Aug 2026 14:54:42 +0200
Subject: [PATCH] Three scale bugs: And gate 32-bit shifts, uninitialized ISS
 cache_sync, requester-side fill responses
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

All three surfaced going past 32 cores, and two of them share one root:
32-bit arithmetic on 64-bit state.

- utils/common_cells/and.cpp: the input words are uint64_t but were set with
  `1 << bit` as an int. Input 31 of a word is INT_MIN and sign-extends into
  bits 31..63 on the OR, so a gate with >= 32 inputs per word can read
  "all true" spuriously; the last-word mask had the same shift and was also
  wrong for input counts that are exact multiples of 64. 64-bit shifts now.
  The icache flush-ack gate has cores+1 inputs: 129 at 32 tiles, 257 at 64.

- cpu/iss exec_inorder.cpp (both): `cache_sync` was cleared only in
  reset(true). The shared icache's own reset fires the flush-ack gate at every
  core, and a core whose reset has not run yet still holds an uninitialized
  cache_sync — true by chance in the debug build — and decrements a zero
  stall counter: the "Trying to decrease zero stalled counter" fatal that
  aborted every traced run at 128 cores and the untraced run at 256. Now
  initialized in the constructor.

- cache/insitu/insitu_cache_xbar.cpp: the requester-side line fill assumed the
  home bank answers synchronously. When the bank's refill path returns
  PENDING the fill stays in flight and its response must come back here, not
  to a core with nothing outstanding. Per-buffer fill requests, a response
  handler on the output ports, a parked waiter for a demand miss that went
  pending, and prefetches that complete in the background.

Verified: rlc_am mu_peak at 32 tiles traced (previously aborted at reset)
reproduces the untraced run's RESULT row to the cycle.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 models/cache/insitu/insitu_cache_xbar.cpp     | 126 +++++++++++++-----
 models/cpu/iss/src/exec/exec_inorder.cpp      |   6 +
 .../cpu/iss/src/snitch_fp_ss/exec_inorder.cpp |   6 +
 models/utils/common_cells/and.cpp             |   9 +-
 4 files changed, 113 insertions(+), 34 deletions(-)

diff --git a/models/cache/insitu/insitu_cache_xbar.cpp b/models/cache/insitu/insitu_cache_xbar.cpp
index 89cf06a1..d241821d 100644
--- a/models/cache/insitu/insitu_cache_xbar.cpp
+++ b/models/cache/insitu/insitu_cache_xbar.cpp
@@ -24,6 +24,7 @@
 #include <cstdint>
 #include <cstdio>
 #include <cstring>
+#include <climits>
 #include <string>
 #include <vector>
 
@@ -69,8 +70,19 @@ private:
         uint64_t line = ~0ull;
         int64_t  ready_cycle = 0;
         std::vector<uint8_t> data;
+        // A fill that went PENDING at the home bank: its request stays in flight and its
+        // response must land HERE — never on a core that has nothing outstanding (the ISS
+        // aborts: "Trying to decrease zero stalled counter"). One fill per buffer; a demand read
+        // that started or met the fill parks as `waiter` and is answered from `fill_resp`.
+        vp::IoReq fill_req;
+        bool      filling = false;
+        uint64_t  fill_addr = ~0ull;
+        vp::IoReq *waiter = nullptr;
+        uint32_t  waiter_off = 0, waiter_n = 0;
     };
-    int64_t fill_line(uint64_t line_addr, uint8_t *dst);
+    enum { FILL_FAILED = -1, FILL_PENDING = -2 };
+    int64_t fill_line(StreamBuf &buf, uint64_t line_addr);
+    static void fill_resp(vp::Block *__this, vp::IoReq *req);
     uint32_t route_out(uint64_t addr, uint64_t *routed_addr);
 
     RouteGeom geom_;
@@ -83,7 +95,7 @@ private:
 
     uint64_t  na_base_ = 0, na_size_ = 0, pf_addr_ = 0;
     uint32_t  line_bytes_ = 64;
-    std::vector<StreamBuf> bufs_;
+    StreamBuf *bufs_ = nullptr;
     uint64_t  cnt_sb_hit_ = 0, cnt_sb_fill_ = 0, cnt_sb_wait_ = 0;
     uint64_t  cnt_pf_ = 0, cnt_pf_late_ = 0, cnt_inval_ = 0;
 
@@ -91,7 +103,6 @@ private:
     std::vector<vp::IoMaster *> outputs_;
     vp::IoSlave config_;
     vp::IoReq split_subreq_;
-    vp::IoReq fill_req_;
     vp::Trace trace_;
 };
 
@@ -131,8 +142,8 @@ InsituCacheXbar::InsituCacheXbar(vp::ComponentConf &conf) : vp::Component(conf)
             pf_addr_ = p;   // 0 when the third field is absent: no prefetch port.
         }
         if (na_size_ != 0) {
-            bufs_.resize(num_cores_);
-            for (auto &buf : bufs_) buf.data.assign(line_bytes_, 0);
+            bufs_ = new StreamBuf[num_cores_];
+            for (uint32_t c = 0; c < num_cores_; ++c) bufs_[c].data.assign(line_bytes_, 0);
         }
     }
 
@@ -145,6 +156,9 @@ InsituCacheXbar::InsituCacheXbar(vp::ComponentConf &conf) : vp::Component(conf)
     }
     for (uint32_t o = 0; o < num_outputs_; ++o) {
         outputs_[o] = new vp::IoMaster();
+        // Responses to requests THIS component issues (the line fills) come back here;
+        // forwarded core requests keep their own originator and are unaffected.
+        outputs_[o]->set_resp_meth(&InsituCacheXbar::fill_resp);
         this->new_master_port("out_" + std::to_string(o), outputs_[o]);
     }
 
@@ -196,21 +210,54 @@ uint32_t InsituCacheXbar::route_out(uint64_t addr, uint64_t *routed_addr)
     return out_id;
 }
 
-// One whole-line fetch through the normal routed path, synchronously; the home bank serves a
-// window read from L2 without installing (insitu_cache_core.cpp). Returns the fetch's latency
-// in cycles, or -1 when the path could not complete synchronously (the caller falls back to
-// forwarding the original request untouched).
-int64_t InsituCacheXbar::fill_line(uint64_t line_addr, uint8_t *dst)
+// One whole-line fetch through the normal routed path; the home bank serves a window read from
+// L2 without installing (insitu_cache_core.cpp). Returns the fetch's latency when it completed in
+// the call, FILL_PENDING when the bank took it asynchronously (the buffer is then `filling`, and
+// fill_resp() finishes it), FILL_FAILED when the path refused it (the caller forwards the original
+// request untouched; no fill is in flight).
+int64_t InsituCacheXbar::fill_line(StreamBuf &buf, uint64_t line_addr)
 {
+    if (buf.filling) return FILL_FAILED;   // one in flight per buffer
     uint64_t routed = line_addr;
     const uint32_t out_id = route_out(line_addr, &routed);
-    fill_req_.init();
-    fill_req_.set_addr(routed);
-    fill_req_.set_size(line_bytes_);
-    fill_req_.set_is_write(false);
-    fill_req_.set_data(dst);
-    if (outputs_[out_id]->req_forward(&fill_req_) != vp::IO_REQ_OK) return -1;
-    return (int64_t)fill_req_.get_full_latency();
+    buf.fill_req.init();
+    buf.fill_req.set_addr(routed);
+    buf.fill_req.set_size(line_bytes_);
+    buf.fill_req.set_is_write(false);
+    buf.fill_req.set_data(buf.data.data());
+    const vp::IoReqStatus st = outputs_[out_id]->req(&buf.fill_req);
+    if (st == vp::IO_REQ_OK) return (int64_t)buf.fill_req.get_full_latency();
+    if (st == vp::IO_REQ_PENDING) {
+        buf.filling = true;
+        buf.fill_addr = line_addr;
+        buf.line = line_addr;
+        buf.ready_cycle = INT64_MAX;   // not before fill_resp
+        return FILL_PENDING;
+    }
+    return FILL_FAILED;
+}
+
+void InsituCacheXbar::fill_resp(vp::Block *__this, vp::IoReq *req)
+{
+    InsituCacheXbar *_this = static_cast<InsituCacheXbar *>(__this);
+    for (uint32_t c = 0; c < _this->num_cores_; ++c) {
+        StreamBuf &buf = _this->bufs_[c];
+        if (&buf.fill_req != req) continue;
+        buf.filling = false;
+        if (buf.line == buf.fill_addr) {
+            buf.ready_cycle = _this->clock.get_cycles();
+        } else {
+            buf.line = ~0ull;   // invalidated by a window write while in flight
+        }
+        if (buf.waiter != nullptr) {
+            vp::IoReq *w = buf.waiter;
+            buf.waiter = nullptr;
+            if (w->get_data() != nullptr)
+                memcpy(w->get_data(), buf.data.data() + buf.waiter_off, buf.waiter_n);
+            w->get_resp_port()->resp(w);
+        }
+        return;
+    }
 }
 
 vp::IoReqStatus InsituCacheXbar::req_handler(vp::Block *__this, vp::IoReq *req, int input_id)
@@ -231,12 +278,12 @@ vp::IoReqStatus InsituCacheXbar::req_handler(vp::Block *__this, vp::IoReq *req,
                 StreamBuf &buf = _this->bufs_[input_id];
                 const uint64_t line = (uint64_t)target & ~((uint64_t)_this->line_bytes_ - 1);
                 if (buf.line != line) {
-                    const int64_t lat = _this->fill_line(line, buf.data.data());
+                    const int64_t lat = _this->fill_line(buf, line);
                     if (lat >= 0) {
                         buf.line = line;
                         buf.ready_cycle = _this->clock.get_cycles() + lat;
-                        _this->cnt_pf_++;
                     }
+                    if (lat != FILL_FAILED) _this->cnt_pf_++;
                 }
             }
             req->inc_latency(1);
@@ -248,8 +295,10 @@ vp::IoReqStatus InsituCacheXbar::req_handler(vp::Block *__this, vp::IoReq *req,
                 // Same-tile window write: drop every local buffer holding the line, then fall
                 // through to the ordinary path (the bank writes through without allocating).
                 const uint64_t line = addr & ~((uint64_t)_this->line_bytes_ - 1);
-                for (auto &buf : _this->bufs_)
+                for (uint32_t c = 0; c < _this->num_cores_; ++c) {
+                    StreamBuf &buf = _this->bufs_[c];
                     if (buf.line == line) { buf.line = ~0ull; _this->cnt_inval_++; }
+                }
             } else {
                 const uint64_t line = addr & ~((uint64_t)_this->line_bytes_ - 1);
                 const uint32_t off = (uint32_t)(addr & (_this->line_bytes_ - 1));
@@ -257,7 +306,15 @@ vp::IoReqStatus InsituCacheXbar::req_handler(vp::Block *__this, vp::IoReq *req,
                 if (off + n <= _this->line_bytes_) {
                     StreamBuf &buf = _this->bufs_[input_id];
                     const int64_t now = _this->clock.get_cycles();
-                    if (buf.line == line) {
+                    if (buf.line == line && buf.filling) {
+                        // A prefetch (or earlier read) is still in flight for this line: park
+                        // behind it if the slot is free, otherwise take the plain path.
+                        if (buf.waiter == nullptr) {
+                            buf.waiter = req; buf.waiter_off = off; buf.waiter_n = n;
+                            _this->cnt_sb_hit_++; _this->cnt_pf_late_++;
+                            return vp::IO_REQ_PENDING;
+                        }
+                    } else if (buf.line == line) {
                         const int64_t wait = (buf.ready_cycle > now) ? buf.ready_cycle - now : 0;
                         if (wait > 0) { _this->cnt_sb_wait_ += (uint64_t)wait; _this->cnt_pf_late_++; }
                         if (req->get_data() != nullptr)
@@ -266,17 +323,24 @@ vp::IoReqStatus InsituCacheXbar::req_handler(vp::Block *__this, vp::IoReq *req,
                         _this->cnt_sb_hit_++;
                         return vp::IO_REQ_OK;
                     }
-                    const int64_t lat = _this->fill_line(line, buf.data.data());
-                    if (lat >= 0) {
-                        buf.line = line;
-                        buf.ready_cycle = now + lat;
-                        if (req->get_data() != nullptr)
-                            memcpy(req->get_data(), buf.data.data() + off, n);
-                        req->inc_latency(_this->xbar_latency_cycles_ + 1 + lat);
-                        _this->cnt_sb_fill_++;
-                        return vp::IO_REQ_OK;
+                    if (!buf.filling) {
+                        const int64_t lat = _this->fill_line(buf, line);
+                        if (lat >= 0) {
+                            buf.line = line;
+                            buf.ready_cycle = now + lat;
+                            if (req->get_data() != nullptr)
+                                memcpy(req->get_data(), buf.data.data() + off, n);
+                            req->inc_latency(_this->xbar_latency_cycles_ + 1 + lat);
+                            _this->cnt_sb_fill_++;
+                            return vp::IO_REQ_OK;
+                        }
+                        if (lat == FILL_PENDING) {
+                            buf.waiter = req; buf.waiter_off = off; buf.waiter_n = n;
+                            _this->cnt_sb_fill_++;
+                            return vp::IO_REQ_PENDING;
+                        }
                     }
-                    // Fill failed (asynchronous path): fall through to plain forwarding.
+                    // Fill refused, or another line's fill occupies the buffer: plain path.
                 }
             }
         }
diff --git a/models/cpu/iss/src/exec/exec_inorder.cpp b/models/cpu/iss/src/exec/exec_inorder.cpp
index a71dae2d..08c54296 100644
--- a/models/cpu/iss/src/exec/exec_inorder.cpp
+++ b/models/cpu/iss/src/exec/exec_inorder.cpp
@@ -43,6 +43,12 @@ void Exec::build()
     this->offload_grant_itf.set_sync_meth(&Exec::offload_grant);
     this->iss.top.new_slave_port("offload_grant", &this->offload_grant_itf, (vp::Block *)this);
 
+    // Initialized here, not only in reset(true): the shared icache's own reset can fire the
+    // flush-ack And gate at every core BEFORE this core's reset has run (reset_all order), and a
+    // stale true here then decrements a zero stall counter — the "Trying to decrease zero
+    // stalled counter" fatal seen at 128+ cores, where reset ordering and uninitialized memory
+    // differ from the small configurations.
+    this->cache_sync = false;
     flush_cache_ack_itf.set_sync_meth(&Exec::flush_cache_ack_sync);
     this->iss.top.new_slave_port("flush_cache_ack", &flush_cache_ack_itf, (vp::Block *)this);
     this->iss.top.new_master_port("flush_cache_req", &flush_cache_req_itf);
diff --git a/models/cpu/iss/src/snitch_fp_ss/exec_inorder.cpp b/models/cpu/iss/src/snitch_fp_ss/exec_inorder.cpp
index 8b5a6959..a2f33b2f 100644
--- a/models/cpu/iss/src/snitch_fp_ss/exec_inorder.cpp
+++ b/models/cpu/iss/src/snitch_fp_ss/exec_inorder.cpp
@@ -42,6 +42,12 @@ void Exec::build()
     this->offload_grant_itf.set_sync_meth(&Exec::offload_grant);
     this->iss.top.new_slave_port("offload_grant", &this->offload_grant_itf, (vp::Block *)this);
 
+    // Initialized here, not only in reset(true): the shared icache's own reset can fire the
+    // flush-ack And gate at every core BEFORE this core's reset has run (reset_all order), and a
+    // stale true here then decrements a zero stall counter — the "Trying to decrease zero
+    // stalled counter" fatal seen at 128+ cores, where reset ordering and uninitialized memory
+    // differ from the small configurations.
+    this->cache_sync = false;
     flush_cache_ack_itf.set_sync_meth(&Exec::flush_cache_ack_sync);
     this->iss.top.new_slave_port("flush_cache_ack", &flush_cache_ack_itf, (vp::Block *)this);
     this->iss.top.new_master_port("flush_cache_req", &flush_cache_req_itf);
diff --git a/models/utils/common_cells/and.cpp b/models/utils/common_cells/and.cpp
index 9d0a50dc..e5c82e89 100644
--- a/models/utils/common_cells/and.cpp
+++ b/models/utils/common_cells/and.cpp
@@ -64,7 +64,10 @@ And::And(vp::ComponentConf &config)
 
     this->nb_values = (nb_input + 63) / 64;
 
-    this->last_value_mask = ~((1 << (nb_input % 64)) - 1);
+    // 64-bit shifts: inputs 31..63 of a word used `1 << bit` as an int — bit 31 is INT_MIN
+    // and sign-extends into all upper bits, so a 129-input gate (128 cores + 1 bank) reads
+    // "all acked" at reset and every core gets a flush-ack it never asked for.
+    this->last_value_mask = (nb_input % 64) ? ~((1ULL << (nb_input % 64)) - 1ULL) : 0ULL;
 
     this->values.resize(this->nb_values);
 }
@@ -95,11 +98,11 @@ void And::sync(vp::Block *__this, bool value, int id)
 
     if (value)
     {
-        _this->values[value_byte] |= 1 << value_bit;
+        _this->values[value_byte] |= 1ULL << value_bit;
     }
     else
     {
-        _this->values[value_byte] &= ~(1 << value_bit);
+        _this->values[value_byte] &= ~(1ULL << value_bit);
     }
 
     if (_this->values[value_byte] == -1)
-- 
2.50.1

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions