Cluster-level Barrier does not update phase information correctly

[RunjiaChen]

Environment Setup

vortex: 5d62846

Bug Description

We noticed that VX_bar_unit advances a phase on a global-barrier response but
commits it to a slot address that the response never supplied, so the global
barrier's own phase is never advanced and an unrelated slot is flipped
instead.

A global barrier occupies a per-core slot in VX_bar_unit in addition to its
row in VX_gbar_unit, and that per-core slot is where its phase bit lives. The
phase is the value gbarrier::arrive() hands out and gbarrier::wait()
consumes — sw/kernel/include/vx_barrier.h:68-78 documents arrive() as
returning the "phase (current generation number)" and wait() as blocking
"until generation > phase". A completed global-barrier generation must
therefore change that value.

On rtlsim it does not change, on any core. Two arrive() calls on the same
global barrier, separated by a completed generation, return the same number.

vortex::gbarrier g(1);
uint32_t p = g.arrive();   // p = 0
do_work();
g.wait(p);                 // releases when phase != 0 -> never happens

Only the async barrier API can observe this. A sync global barrier
(arrive_and_wait / vx.bar) releases through unlock_mask and never
compares a phase, so the missing advance is invisible there.

The async arrive()/wait() pair on a global barrier does not have a working
release mechanism of its own. wait() parks the warp until the phase differs
from its token (VX_bar_unit.sv:155) and the phase never differs, so what
actually releases those warps is the response arm's unconditional broadcast on
the same cycle it mis-writes the phase:

166:  if (gbar_bus_if.rsp_valid && gbar_rsp_ready && (gbar_bus_if.rsp_data.id == gbar_req_id_r)) begin
167:      unlock_valid_n = 1;         // release stalled warps
168:      unlock_mask_n = active_warps; // ALL active warps, phase not consulted
169:      phase_n = next_phase;
170:  end

That broadcast only helps a warp that is already parked when the response
is consumed. A warp that reaches wait() after the response has been taken has
nothing left to release it — the response is a one-shot handshake, and the next
one only comes with the next generation of that barrier. So async global
barriers currently work by timing rather than by the documented phase
protocol.

The in-tree tests/regression/async_gbarrier passes on both drivers at
5d62846c6, so it does not catch this. It passes on timing: its kernel does 32
loop iterations between bar1.arrive() (:25) and bar1.wait(phase) (:33),
which is short enough that every warp is already parked when the response is
consumed, so the broadcast at :168 releases them all.

Widening that window is enough to turn it into a hard deadlock. Changing only
the trip count at tests/regression/async_gbarrier/kernel.cpp:28:

-  for (uint32_t i = 0; i < 32; ++i) {
+  for (uint32_t i = 0; i < 2000; ++i) {

--driver=simx still passes (instrs=96610, cycles=127654, PASSED!).
--driver=rtlsim deadlocks, caught by the scheduler's own watchdog:

[449633] %Error: VX_scheduler.sv:598: Assertion failed in TOP.rtlsim_shim.vortex.g_clusters[0].cluster.g_sockets[1].socket.g_cores[0].core.scheduler: *** cluster0-socket1-core0-scheduler timeout: active_warps=1111, stalled_warps=1111
%Error: /work/vortex/hw/rtl/core/VX_scheduler.sv:598: Verilog $stop

All four warps active, all four stalled: every warp reached wait() after the
response had been taken, the phase compare at :155 was false because the
phase never advanced, and the one-shot broadcast was already spent. For a warp
parked on a global barrier these are the only two release paths in the
module — unlock_valid_n is assigned at :89/:99/:115 only inside the
~req_data.is_global arm, leaving :156 (phase, broken) and :167
(broadcast, one-shot).

We are not proposing that widened loop as a test; a deadlocking test is a poor
CI citizen. We mention it because it is a one-constant change to a file already
in the tree, and it shows the current pass is not evidence that the phase
protocol works.

Below is our regression test. Please place it in the tests/regression/
directory. Then execute:
./ci/blackbox.sh --cores=2 --app=gbar_phase --driver=rtlsim
We obtain:

CONFIGS: num_threads=4, num_warps=4, num_cores=2, num_clusters=1, socket_size=1, local_mem_base=0x1ffff0000, num_barriers=8
num_cores=2, num_warps=4, num_threads=4
num_groups=8
allocate device memory
load kernel module
start device
download results
wait for completion

core  phase@N  phase@N+1  advanced
  0        0          0        NO
    core 0: PHASE error: a completed global-barrier generation did not advance the phase (0 -> 0)
  1        0          0        NO
    core 1: PHASE error: a completed global-barrier generation did not advance the phase (0 -> 0)
phase errors: 2
cleanup
PERF: instrs=297, cycles=772, IPC=0.385
Found 2 errors!
FAILED!
make: *** [../common.mk:222: run-rtlsim] Error 2

Further cross-validation with simx:
./ci/blackbox.sh --cores=2 --app=gbar_phase --driver=simx
We obtain:

num_cores=2, num_warps=4, num_threads=4
num_groups=8
allocate device memory
load kernel module
start device
download results
wait for completion

core  phase@N  phase@N+1  advanced
  0        0          1        yes
  1        0          1        yes
phase errors: 0
cleanup
PERF: instrs=290, cycles=844, IPC=0.344
PASSED!

Deterministic, not intermittent, and identical on both cores. SimX advances
the phase on the cluster release (sim/simx/barrier_unit.cpp:126,
++barrier.phase at the end of global_resume) and is unaffected, which is
also what shows the audit itself is sound.

The test samples the phase with arrive() rather than blocking in wait(), on
purpose: arrive() is the non-blocking half of the pair, so the kernel always
terminates and the defect is reported as data instead of a timeout. It arrives
once on a probe barrier (every warp of every core, so the cluster does
release), rendezvouses on a second global barrier to guarantee the release has
landed everywhere, then arrives again and compares. The result is reported per
core because a global barrier is a cluster object whose phase lives in per-core
state.

Patch

We found that the root cause is that VX_bar_unit is address-driven one cycle
ahead of the request, and a global-barrier response has no address to drive.

read_addr is documented "valid one cycle before req_valid" (:27). It feeds
the store read port directly (:182), and the write address is that same
address delayed by one cycle (:238), so a request's read at T and its
write-back at T+1 land on the same slot:

27:   input wire [BAR_ADDR_W-1:0] read_addr, // valid one cycle before req_valid
182:  wire [BAR_ADDR_W-1:0] store_raddr = read_addr;
238:  store_waddr <= store_raddr;

That contract holds only for events originating from an instruction in
Execute, because the address is produced combinationally from rs1:

VX_wctl_unit.sv:140  wire wctl_bar_enable = wctl_valid && is_bar;
VX_wctl_unit.sv:177  assign warp_ctl_if.bar_addr = wctl_bar_enable ? wctl_bar_addr
                                                                   : txbar_bus_if.data.addr;
VX_scheduler.sv:385      .read_addr  (warp_ctl_if.bar_addr),

A global-barrier response does not originate from an instruction, and it is
accepted only when no request is present:

57:   wire gbar_rsp_ready = ~req_valid;

so by construction no barrier instruction supplied an address on the cycle a
response is taken. bar_addr falls through to txbar_bus_if.data.addr, which
without DXA is tied to 'x (VX_sfu_unit.sv:224).

The response arm writes anyway:

166:  if (gbar_bus_if.rsp_valid && gbar_rsp_ready && (gbar_bus_if.rsp_data.id == gbar_req_id_r)) begin
169:      phase_n = next_phase; // advance phase
170:  end
185:  wire                  store_phase_wdata = phase_n;
186:  wire                  store_write = req_valid || gbar_bus_if.rsp_valid;

next_phase is ~phase_r (:61) and phase_r holds the phase of
store_waddr (:236), not of the global barrier. So the complement is taken
of the wrong slot's phase, and store_write then commits it to store_waddr,
also the wrong slot. Two errors compound: the global barrier's slot is never
written, and an unrelated slot's phase is flipped. The second half is the more
dangerous one — if that slot belongs to a local async barrier, that barrier
releases a generation early or hangs, with no global barrier anywhere near the
code that breaks.

Which slot it lands on is read_addr from the preceding cycle. In a
2-state build the 'x resolves to slot 0; under --x-assign unique it varies
per build, and we have measured it landing on four different slots across
builds of the same source.

The unit already latches what a response needs in order to be routed:
gbar_req_id_r is captured when the request is forwarded (:270) and matched
against the response at :166. The slot address and its phase are simply not
captured alongside it. The fix direction we would suggest is to latch both at
forward time and use them on the response — a response should advance the phase
of the slot that issued the request, and should not touch the state RAM at all
(the mask/count/events were already cleared at forward time, :145):

// captured on the rising edge of gbar_req_valid, when store_waddr and phase_r
// still belong to the arriving global request
gbar_slot_addr_r  <= store_waddr;
gbar_slot_phase_r <= phase_r;

// response arm
phase_n = ~gbar_slot_phase_r;

// store write
wire gbar_rsp_fire      = USE_GBAR && gbar_bus_if.rsp_valid && gbar_rsp_ready;
wire store_state_write  = req_valid;                        // requests only
wire store_phase_write  = req_valid || gbar_rsp_fire;
wire [BAR_ADDR_W-1:0] store_phase_waddr = gbar_rsp_fire ? gbar_slot_addr_r
                                                        : store_waddr;

We have not verified this sketch in simulation, and we have not evaluated
timing or area impact. It also overlaps the unguarded phase_r reload at
:236 reported separately in the bar_slot_phase issue; a complete fix
probably wants both, since :236 is the path by which a response's phase write
also lands in the working register.

The test is on this branch, based directly on 5d62846c6:
https://github.com/RunjiaChen/vortex-visualiser/tree/bug/gbar-response-phase
It adds one directory and modifies no existing file. It is deliberately not
added to the TESTS list in tests/regression/Makefile, so CI is unaffected
by a test that is expected to fail until a fix lands. It requires at least two
cores for the global barrier hardware to be instantiated
(USE_GBAR = VX_CFG_NUM_CORES > 1, VX_bar_unit.sv:44); the Makefile forces
-DVX_CFG_NUM_CORES=2 and the host program exits with a skip message on a
single-core device.

Also present, byte-identical, at d76b7f24e. Between the two commits exactly
one touches VX_bar_unit.sv — 90a9b186b, whose entire diff for that file is
one word inside a comment — and VX_gbar_unit.sv is untouched.