PASS: Completed Review #6 | research/05-memory/atomics.md

research/.xh1/runs/20260826T142322Z/review.md

@@ -0,0 +1,82 @@
+## Independent Technical Review – XH-1 Memory Atomics & Consistency Model
+
+I have reviewed the document `research/05-memory/atomics.md` (Revision 2.0) as an independent technical reviewer. The document contains **multiple substantive errors** that prevent it from being accepted in its current form. Four of these are critical because they involve RISC‑V specification violations, internal contradictions, or unsupported quantitative claims. The remaining issues are significant but subsidiary.
+
+### Critical Issues
+
+1. **Contradictory memory‑model claim (Ztso vs. RVWMO)**
+   The document states that XH‑1 implements the Ztso extension **and** retains RVWMO as the baseline. The RISC‑V architecture defines Ztso as a memory‑model extension that replaces the default RVWMO with a Total Store Order (TSO) model. Implementing Ztso means the core **must** follow TSO; it cannot simultaneously adhere to RVWMO. This is a hard specification error and an internal contradiction.
+
+2. **Unsupported atomic‑throughput target**
+   “Peak atomic throughput: ≥ 400M ops/sec/core under uniform distribution” is presented as a target but is never justified. For 128 cores this would be >51 billion atomic operations per second, roughly one per core per cycle in a multi‑GHz design. No evidence, simulation data, or architectural analysis supports this claim, and it is highly unrealistic for a coherent‑memory system.
+
+3. **Incorrect FENCE mapping**
+   The document maps `FENCE.RI` to a “load‑fetch drain”. In RISC‑V, `FENCE` uses predecessor/successor sets with bits `I`, `O`, `R`, `W`. `FENCE.RI` orders reads before device **input** operations, not instruction fetches. Instruction‑fetch fencing is provided by the separate `FENCE.I` instruction. This is a clear misreading of the specification.
+
+4. **Misleading description of FENCE.TSO “emulation”**
+   The text claims an “optional FENCE.TSO emulation layer provides TSO‑like semantics for legacy binaries without modifying the base pipeline.” The Ztso extension already defines a full TSO memory model; `FENCE.TSO` is a specific barrier instruction, not a stand‑alone emulation layer. Moreover, if Ztso is implemented, the memory model is TSO and the “emulation” phrasing is inappropriate. The claim conflates the extension with a single instruction and mischaracterises the hardware support.
+
+### Additional Issues
+
+5. **Unsourced benchmark projections**
+   Section 5.2 presents a table of “Expected Speedup” vs. 32‑core for various workloads. There is no indication of methodology, modelling, simulation, or analytical basis. The numbers are presented as facts without supporting evidence.
+
+6. **Half‑word/byte AMO variants**
+   The document states support for “Half‑word and byte variants where applicable” of AMO instructions. The standard RISC‑V A extension defines only word and double‑word AMOs (for RV64). If the team intends to implement custom byte/half‑word AMOs this must be explicitly stated as a non‑standard extension; otherwise it is a specification error.
+
+7. **Inappropriate citation**
+   The AMD64 Architecture Programmer’s Manual (Vol. 2) is cited as a source for “Directory vs. snooping trade‑offs”. That manual is a programmer’s reference for x86‑64 memory ordering and does not contain cache‑coherence design trade‑offs. The citation is invalid.
+
+8. **Misleading “measured” claim**
+   “Contention degradation: < 15% throughput loss at 75% saturation (measured via synthetic benchmarks)” – since the processor does not exist, this cannot be a measurement; it is at best a simulation result. The wording should reflect that clearly.
+
+### Required Fixes
+
+The document must be corrected before it can be accepted. The following changes are mandatory:
+
+- **Resolve the Ztso/RVWMO contradiction.** Decide whether XH‑1 implements RVWMO (default) or the Ztso extension (TSO). The two are mutually exclusive. Update all sections accordingly and remove any conflicting statements. If Ztso is chosen, the memory model section must describe TSO, not RVWMO, and the claim of “retaining RVWMO as the baseline” must be deleted.
+
+- **Justify or remove the atomic‑throughput target.** Either provide a detailed microarchitectural analysis, simulation results, or a credible analytical model that supports the 400M‑ops/sec/core figure, or reduce it to a realistic value with clear reasoning. The target must be flagged as an assumption or design goal with appropriate caveats.
+
+- **Correct the FENCE.RI/FENCE.WI mapping.** Remove the incorrect “load‑fetch drain” and “store‑fetch drain” descriptions and replace them with the correct semantics per the RISC‑V specification (R‑before‑I, W‑before‑I, etc.). The only instruction‑fetch fence is `FENCE.I`.
+
+- **Clarify the Ztso/FENCE.TSO support.** If Ztso is implemented, describe the memory model as TSO, not as an “emulation layer”. If Ztso is not implemented, the optional `FENCE.TSO` emulation must be described as a software‑inserted barrier strategy, not as a hardware‑provided TSO mode. The current text is ambiguous and misleading.
+
+- **Document the basis for benchmark projections.** State the methodology (e.g., analytical model, simulation, extrapolation) and any assumptions. If the numbers are purely aspirational, label them as such and do not present them as “Expected Speedup”.
+
+- **Address the half‑word/byte AMO claim.** Either remove the claim or explicitly state that XH‑1 will implement custom byte/half‑word AMOs as a non‑standard extension, with a note on compatibility and software impact.
+
+- **Replace the invalid AMD citation.** Cite an appropriate source for cache‑coherence trade‑offs (e.g., a computer architecture textbook, research paper, or the original directory‑protocol literature). Alternatively, remove the reference.
+
+- **Change “measured” to “projected” or “simulated”** in the contention‑degradation statement, and note the nature of the underlying evaluation.
+
+### Verdict
+
+**FAIL** – The document contains critical specification contradictions, unsupported quantitative claims, and incorrect RISC‑V semantics that must be addressed before it is suitable for further research planning.
+
+```json
+{
+  "verdict": "FAIL",
+  "confidence": "HIGH",
+  "issues": [
+    "Contradictory memory model: XH‑1 implements Ztso (TSO) but claims RVWMO as baseline; the two are mutually exclusive per RISC‑V specification.",
+    "Unsupported atomic‑throughput target of ≥400M ops/sec/core – no evidence or analysis provided, and the figure is unrealistic for a 128‑core coherent system.",
+    "Incorrect mapping of FENCE.RI/FENCE.WI to “load‑fetch drain” and “store‑fetch drain”; the I bit in FENCE refers to device input, not instruction fetch.",
+    "Misleading description of FENCE.TSO emulation; conflates the Ztso extension (which changes the memory model) with a single barrier instruction, and implies a hardware TSO layer not consistent with the chosen model.",
+    "Benchmark projections (Section 5.2) are unsupported – no methodology, simulation data, or analytical basis is given.",
+    "Claim of half‑word and byte AMO variants is not standard RISC‑V; if intended as a custom extension this must be stated explicitly.",
+    "Invalid citation of AMD64 Architecture Programmer’s Manual for directory vs. snooping trade‑offs.",
+    "Contention degradation stated as “measured” while no silicon exists; should be labelled as projected/simulated."
+  ],
+  "required_fixes": [
+    "Decide and clearly state whether XH‑1 implements RVWMO or Ztso; remove all contradictory statements and align the entire memory‑model description.",
+    "Provide a credible justification for the atomic‑throughput target or reduce it to a realistic value with explicit assumptions.",
+    "Correct the FENCE.RI/FENCE.WI semantics to the RISC‑V specification (R‑before‑I, W‑before‑I, etc.) and remove the erroneous “load‑fetch” and “store‑fetch” terminology.",
+    "Clarify the Ztso/FENCE.TSO support: if Ztso is implemented, describe the TSO memory model; if not, explain the FENCE.TSO barrier as a software‑inserted mechanism and avoid the term “emulation layer”.",
+    "Document the basis for all benchmark projections (e.g., simulation, analytical model) and label them as projected or aspirational.",
+    "Remove the claim of half‑word/byte AMOs or explicitly state that they are a custom, non‑standard extension.",
+    "Replace the invalid AMD citation with a proper reference on cache‑coherence trade‑offs or remove it.",
+    "Change “measured” to “projected” or “simulated” in the contention‑degradation statement."
+  ]
+}
+```