Fix || vs && mismatch in order_book.sv latency latch#4
Open
pauliano22 wants to merge 1 commit into
Open
Conversation
The comment above this line says latency should be recorded "to when
both sides are valid", and book_valid (line 170) correctly uses &&
for that same condition. This line used ||, so r_latency was
overwritten on almost every Add message instead of only once both
sides first became valid.
Currently inert: top.sv wires ob_latency_cycles but explicitly
ignores it ("unused; top counts separately"), so this doesn't affect
any latency numbers reported today - but it's a real bug that would
produce wrong values the moment this signal is used.
Verified: `verilator --lint-only` clean, and a 50K-message run through
sim/verilator/obj_dir/Vtop produces identical throughput, latency
percentiles, and golden-model comparison counts before and after
(expected, since this only changes an unused internal register).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/rtl/order_book.sv'sr_latencyupdate was gated onr_best_bid != 0 || r_best_ask != 0, but the comment directly above it says latency should be recorded "to when both sides are valid" — andbook_valid(a few lines down) correctly uses&&for that same condition. With||,r_latencygets overwritten on almost every Add message rather than only once both sides first become valid.This is currently inert:
top.svwiresorder_book'slatency_cyclesoutput toob_latency_cyclesbut explicitly ignores it (comment:// unused; top counts separately). So no reported number changes today, but it's a genuine bug that would produce wrong values the moment this signal is actually consumed.Test plan
verilator --lint-only -Wall -y ../../src/rtl ../../src/rtl/top.sv— cleansim/verilatorand ran./obj_dir/Vtop --file ../../data/sample.itch --no-vcd --limit 50000before and after this change — identical throughput, latency percentiles (p50/p95/p99/max), and golden-model comparison counts in both cases, confirming this only touches the currently-unused internal register and doesn't regress anythingGenerated by Claude Code