Adding fence to the end of sync() operation in tile group barrier

[bornaehsani]

Cosim regression passes with 4x4_fast_n_fake, 4x4_blocking_vcache_f1_model, timing_v0_8_4.
TODO: Add a regression test that violates read-after-write fence, but works with this fix as Bandhav suggested.

[vb000]

I wonder if sync operation should start with fence rather than end with fence... Tiles that finish executing fence fast might read stale values, if it comes after syncing.

[vb000]

A regression test that violates read-after-write without fence, but works with this fix might be a good addition to barrier test...

[taylor-bsg]

I agree with both comments by Bandhav.

Borna & all, please take extra care when you are writing synchronization code to simulate in your head or on paper what is happening in the machine and how changing arrival time of messages could result in unexpected execution. You don’t get this kind of code right by changing it and seeing if it works — you may just have gotten lucky. You need to prove that it is correct instead.

[tommydcjung]

Will there be a minimal spmd test to reproduce the bug?

[bornaehsani]

Not sure how to reproduce this, hoping to discuss it in the meeting tomorrow.

[drichmond]

Try editing a variable in memory before and after the barrier. See if GCC or LLVM coalesces the two writes into the single later write. (edit: Pre-fix)

[drichmond]

You may need some sort of no-alias attribute on the memory you're trying to write

[drichmond]

@vb000 Did you have a working example where this bug appeared?

[vb000]

I've an example that logically needs a fence, but program doesn't fail without one because remote stores are faster than barrier in this case (and in most cases I think).

[vb000]

FWIW, I'm not sure if this is a bug, but in practice, we have come to expect the barrier to also include fence. Even my initial instinct was to find some mechanism to make sure stores are completed, because I thought bsg_barrier would just be a program barrier.

[bornaehsani]

I created an issue regarding the need for a regression test that exposes the bug when barriers do not have fence (issue #371), in the meantime can we have this PR merged to avoid future bugs?

[drichmond]

This would be easier with #370, OR by refactoring the barrier so that the write addresses are determined at initialization (not sync() time). Both would speed up the sync() operation.

Here's a situation that could expose a read-after-write failure.

Imagine a large (ish) tile group, like 8x8. Imagine that all 8 tiles in the right column storm the top vcache with read-misses and writes. The addresses don't matter, what matters is that there are misses that stall the vcache AND there is congestion that stalls the vertical network of that column (but not the tiles in that column)

Then the bottom right tile issues a write.

This is followed by a barrier.

Then the top-right tile issues a read.

Since all of the tiles have issued their reads/writes, and sync() is horizontal communication for all but the center column, the sync will succeed. If the sync() operation is fast enough, then the tiles will finish sync-ing before the bottom-right-write reaches the top-right vcache. Then a read-after-write hazard could be exposed by the top-right tile reading the address.

This would be easier to expose on long-latency memory (HBM), a small vcache request buffer, and on a large tile group.

The hardware fence operation absolutely needs to happen at the end of the sync() method. Repeated syncs() without the center/origin tiles knowing that the previous sync() writes have finished could cause issues (I can construct a situation like above if needed).

I'm still thinking about whether it needs to happen at the START.