Repository navigation
mvins that write the same scratchpad/accumulator rows can complete out of order #409
Description
Two mvins to overlapping rows, without a fence between them, can complete in the wrong order, so the rows end up with the older mvin's data. This affects a zero mvin (DRAM address 0) after a normal mvin, and two normal mvins into the accumulator (overwrite, then accumulate). No assertion fires.
Environment: Gemmini 8c3f9923 (Chipyard 1.15.0), Verilator 5.022, GemminiRocketConfig SoC with Gemmini DIM = 4 and DIM = 8 (INT8/INT32, Dataflow.BOTH). Tests built like the bareMetalC tests and run with make -C sims/verilator CONFIG=<config> LOADMEM=1 RANDOM_SEED=1 run-binary-fast BINARY=<elf>. Reference: Spike with libgemmini.
Reproducer 1: zero mvin after an mvin to the same rows
waw_min.c
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include "include/gemmini_testutils.h"
static elem_t In[DIM][DIM] row_align(1);
static elem_t Out[DIM][DIM] row_align(1);
int main() {
for (int i = 0; i < DIM; i++)
for (int j = 0; j < DIM; j++)
In[i][j] = 1 + i * DIM + j;
gemmini_flush(0);
gemmini_config_ld(DIM * sizeof(elem_t));
gemmini_config_st(DIM * sizeof(elem_t));
gemmini_mvin(In, 0); // rows 0..DIM-1 <- In
gemmini_mvin(NULL, 0); // rows 0..DIM-1 <- zeros
gemmini_mvout(Out, 0);
gemmini_fence();
int bad = 0;
for (int i = 0; i < DIM; i++)
for (int j = 0; j < DIM; j++)
bad += Out[i][j] != 0;
printf("%d of %d elements are not zero\n", bad, DIM * DIM);
if (bad)
printMatrix(Out);
exit(bad != 0);
}- Expected (Spike):
0 of 16 elements are not zero. - Observed (RTL):
16 of 16 elements are not zeroatDIM = 4, andOutequalsIn; 64 of 64 atDIM = 8. - With
gemmini_fence()between the two mvins, or with the mvins swapped, the result is right; aconfig_ldin between does not help. The same happens in the accumulator, and with 64-column block mvins part of the elements keep the old data (92 of 256 atDIM = 4).
Reproducer 2: overwrite, then accumulate, into the same accumulator rows
A is never touched by the CPU, so with LOADMEM=1 its first read misses in the L2, while B is pulled into the L2 first. (This one depends on that timing: loaded over TSI, without LOADMEM, it printed the right result; reproducer 1 fails either way.)
waw_acc_min.c
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include "include/gemmini_testutils.h"
static acc_t A[DIM][16] __attribute__((aligned(64))) = {[0 ... DIM - 1] = {[0 ... 15] = 1}};
static acc_t B[DIM][16] __attribute__((aligned(64))) = {[0 ... DIM - 1] = {[0 ... 15] = 2}};
static elem_t Out[DIM][DIM] row_align(1);
int main() {
gemmini_flush(0);
gemmini_config_ld(16 * sizeof(acc_t));
gemmini_config_st(DIM * sizeof(elem_t));
gemmini_extended_mvin(B, 0x80000000 | 512, DIM, DIM); // pull B into the L2, and wait
gemmini_fence();
gemmini_extended_mvin(A, 0x80000000, DIM, DIM); // acc rows 0..DIM-1 = A
gemmini_extended_mvin(B, 0xC0000000, DIM, DIM); // acc rows 0..DIM-1 += B
gemmini_mvout(Out, 0x80000000);
gemmini_fence();
int bad = 0;
for (int i = 0; i < DIM; i++)
for (int j = 0; j < DIM; j++)
bad += Out[i][j] != 3;
printf("%d of %d elements are not 3\n", bad, DIM * DIM);
if (bad)
printMatrix(Out);
exit(bad != 0);
}- Expected (Spike):
0 of 16 elements are not 3. - Observed (RTL):
16 of 16 elements are not 3atDIM = 4, every element is 1 (the accumulation ofBis lost); 64 of 64 atDIM = 8. - With an uncached
A, the instruction sequence ofbareMetalC/matrix_add.c(atDIM = 4) andtiled_resadd_auto(DIM, DIM, ...)giveC == Ain the same way.
Cause
ReservationStation.scala:327-329: a load depends on older loads only until they are issued, and same-queue dependencies are cleared at issue (:469-470):
when (is_load) {
// war/waw (after ex) | war/waw (after st)
new_entry.deps_ld := VecInit(entries_ld.map { e => e.valid && !e.bits.issued }) // same qSo the second mvin is issued as soon as the first one has been handed to the LoadController, before the first one has written its rows, and the two complete in whatever order their data arrive:
- a zero mvin (
LoadController.scala:72,all_zeros) bypasses the DMA reader and goes to theZeroWriter(Scratchpad.scala:362-371), which writes within a few cycles, while the older mvin still waits for its TileLink responses, which then overwrite the zeros; - the DMA reader writes responses in arrival order (
DMA.scala:292-297), so an mvin whose reads hit in the L2 can finish before an older one whose reads miss.
LoadController.scala:11 still says // TODO we need to check for WAW errors here.
#67 reported the accumulator case and was closed after #73 added a load-to-load check; that check is no longer present since #178 split the reservation station, so the case of #67 fails again on current master.
Suggested fix
Keep a load's dependency on an older load with an overlapping destination until that load completes (first hunk: the older load is already issued when the new one is allocated; second hunk: it is issued later).
diff
--- a/src/main/scala/gemmini/ReservationStation.scala
+++ b/src/main/scala/gemmini/ReservationStation.scala
@@ -326,7 +326,10 @@ class ReservationStation[T <: Data : Arithmetic, U <: Data, V <: Data](config: G
val not_config = !new_entry.is_config
when (is_load) {
// war/waw (after ex) | war/waw (after st)
- new_entry.deps_ld := VecInit(entries_ld.map { e => e.valid && !e.bits.issued }) // same q
+ // Loads can complete out of order (zero-mvins bypass the DMA reader, and TileLink responses can return out of
+ // order), so a load must also wait for an older, already issued load that writes the same rows (waw)
+ new_entry.deps_ld := VecInit(entries_ld.map { e => e.valid && (!e.bits.issued ||
+ (not_config && e.bits.opa.valid && new_entry.opa.bits.overlaps(e.bits.opa.bits))) }) // same q
new_entry.deps_ex := VecInit(entries_ex.map { e => e.valid && !new_entry.is_config && (
(new_entry.opa.bits.overlaps(e.bits.opa.bits) && e.bits.opa.valid) || // waw if preload, war if compute
@@ -467,7 +470,15 @@ class ReservationStation[T <: Data : Arithmetic, U <: Data, V <: Data](config: G
entries_type_.zipWithIndex.foreach { case (e, i) =>
val deps_type = if (q == ldq) e.bits.deps_ld else if (q == exq) e.bits.deps_ex else e.bits.deps_st
if ((q == q_) && (q_ != stq)) {
- deps_type(issue_id) := false.B // TODO(richard): normal mvouts should not be blocked until complete
+ // a load keeps its dependency on an older load to the same rows until that load completes (waw)
+ val keep_waw = if (q == ldq) {
+ val e_bits = Mux(new_allocs_oh_ld(i), new_entry, e.bits)
+ !e_bits.is_config && e_bits.opa.valid && issue_entry.bits.opa.valid &&
+ e_bits.opa.bits.overlaps(issue_entry.bits.opa.bits)
+ } else false.B
+ when (!keep_waw) {
+ deps_type(issue_id) := false.B // TODO(richard): normal mvouts should not be blocked until complete
+ }
} else {
when (issue_entry.bits.complete_on_issue) {
deps_type(issue_id) := false.BWith this change (tested at DIM = 4) both reproducers and the other cases above give the right result, and 23 bareMetalC tests still pass (18 with unchanged cycle counts; matrix_add, global_average, conv_dw, conv_stride and conv_first_layer take a few hundred to a few thousand more cycles).
Activity
[-]mvins that write the same scratchpad/accumulator rows can complete out of order (again, as in #67)[/-][+]mvins that write the same scratchpad/accumulator rows can complete out of order[/+]