refactor(SBuffer, StoreDifftest): move diff store#850
refactor(SBuffer, StoreDifftest): move diff store#850skyhgzsh wants to merge 1 commit intoOpenXiangShan:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors DiffTest store-event handling by moving store event “calculation/normalization” logic from the Scala side into the C++ checker, along with a breaking change to the DifftestStoreEvent/StoreEvent payload shape.
Changes:
- Replace pre-computed store event fields (
addr/data/mask/eew/...) with raw fields and flags inStoreEvent(Scala bundle). - Recompute effective store address/data/mask in
StoreRecorder::check()(C++) based on the new raw fields. - Adjust vector-store commit splitting to use the newly computed “VSLine” classification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/test/csrc/difftest/checkers/store.cpp | Adds C++-side store-event normalization logic (mask expansion, address/data/mask derivation) and updates commit splitting inputs. |
| src/main/scala/Bundles.scala | Redefines StoreEvent bundle fields to carry raw store info and control flags instead of precomputed commit fields (breaking ABI change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ab67eee to
0a1a0f3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
event calculate part from scala to cpp BREAKING CHANGE: struct DifftestStoreEvent is changed and broke backward compatibility!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (calcIsVSLine) { | ||
| uint16_t flow = COMMITBYTES / eew; | ||
| uint64_t flowMask = (eew == 1) ? 0x1ULL : (eew == 2) ? 0x3ULL : (eew == 4) ? 0xfULL : (eew == 8) ? 0xffULL : 0x0ULL; | ||
| uint64_t flowMaskBit = (eew == 1) ? 0xffULL |
There was a problem hiding this comment.
Changing the vector-store path to if (calcIsVSLine) makes this block run whenever calcIsVSLine is true. In the downstream handleMisalign handling inside this block, refStoreCommitData is computed via shifts of (64 - (rawOffset * 8)); when offset % 8 == 0 (so rawOffset == 0), that code shifts by 64 bits, which is undefined behavior in C++. Please guard rawOffset == 0 (result should be 0) or rewrite the extraction using a mask to avoid shift-by-64 UB.
event calculate part from scala to cpp
BREAKING CHANGE: struct DifftestStoreEvent is
changed and broke backward compatibility!