Compare only W0 bank hashes in BTrace

src/nodes/bemu/native/ffi/callbacks.rs

@@ -32,7 +32,8 @@ pub extern "C" fn buckyball_exec(state: *mut c_void, funct7: u8, xs1: u64, xs2:
     let trace = &mut state.trace as *mut TraceState;
     let enable = funct7 >> 4;
     let btrace = state.trace.btrace_enabled()
-        && matches!(enable, 1..=4)
+        && pc != 0
+        && matches!(enable, 2..=4)
         && !matches!(funct7 as u32, FUNCT7_MSET | FUNCT7_MVIN_MMIO);
     if btrace {
         state.bank_scoreboard.issue(inst_id);
@@ -117,64 +118,46 @@ pub extern "C" fn buckyball_exec(state: *mut c_void, funct7: u8, xs1: u64, xs2:
     if btrace {
         bank_scoreboard.complete(inst_id);
         let op_type = format!("funct7_{}", funct7);
-        let r0_enabled = matches!(enable, 1 | 3 | 4);
-        let r1_enabled = enable == 4;
-        let w0_enabled = matches!(enable, 2 | 3 | 4);
-        let r0_vbank = (xs1 & 0x3ff) as u32;
-        let r1_vbank = ((xs1 >> 10) & 0x3ff) as u32;
         let w0_vbank = ((xs1 >> 20) & 0x3ff) as u32;
-        let mut hashes = std::collections::BTreeMap::new();
-        for (enabled, vbank_id) in [(r0_enabled, r0_vbank), (r1_enabled, r1_vbank), (w0_enabled, w0_vbank)] {
-            if !enabled {
-                continue;
-            }
-            if hashes.contains_key(&vbank_id) {
-                continue;
-            }
-            let mut status_hash = 0;
-            let cols = if crate::config::is_shared_vbank(vbank_id as u64) {
+        let mut status_hash = 0;
+        let cols = if crate::config::is_shared_vbank(w0_vbank as u64) {
+            let shared = shared_memory
+                .as_ref()
+                .expect("shared bank storage is unavailable")
+                .banks_mut();
+            let core = *hart_id % (shared.cfgs.len() / shared.virtual_bank_count);
+            shared.cfgs[core * shared.virtual_bank_count + w0_vbank as usize].cols
+        } else {
+            bank_cfgs[w0_vbank as usize].cols
+        };
+        for group_id in 0..cols as u32 {
+            let physical_hash = if crate::config::is_shared_vbank(w0_vbank as u64) {
                 let shared = shared_memory
                     .as_ref()
                     .expect("shared bank storage is unavailable")
                     .banks_mut();
-                let core = *hart_id % (shared.cfgs.len() / shared.virtual_bank_count);
-                shared.cfgs[core * shared.virtual_bank_count + vbank_id as usize].cols
+                let pbank_id = shared
+                    .map
+                    .resolve_hart_group(*hart_id, w0_vbank, group_id)
+                    .unwrap_or_else(|| panic!("unmapped shared vbank {w0_vbank} group {group_id}"));
+                shared.storage[pbank_id].status_hash()
             } else {
-                bank_cfgs[vbank_id as usize].cols
+                let pbank_id = bank_map
+                    .resolve_group(w0_vbank, group_id)
+                    .unwrap_or_else(|| panic!("unmapped vbank {w0_vbank} group {group_id}"));
+                banks[pbank_id].status_hash()
             };
-            for group_id in 0..cols as u32 {
-                let physical_hash = if crate::config::is_shared_vbank(vbank_id as u64) {
-                    let shared = shared_memory
-                        .as_ref()
-                        .expect("shared bank storage is unavailable")
-                        .banks_mut();
-                    let pbank_id = shared
-                        .map
-                        .resolve_hart_group(*hart_id, vbank_id, group_id)
-                        .unwrap_or_else(|| panic!("unmapped shared vbank {vbank_id} group {group_id}"));
-                    shared.storage[pbank_id].status_hash()
-                } else {
-                    let pbank_id = bank_map
-                        .resolve_group(vbank_id, group_id)
-                        .unwrap_or_else(|| panic!("unmapped vbank {vbank_id} group {group_id}"));
-                    banks[pbank_id].status_hash()
-                };
-                status_hash = combine_bank_hash(status_hash, group_id, physical_hash);
-            }
-            hashes.insert(vbank_id, status_hash);
+            status_hash = combine_bank_hash(status_hash, group_id, physical_hash);
         }
-        let slot = |enabled: bool, vbank_id: u32| BTraceBank {
-            vbank_id: if enabled { vbank_id } else { INVALID_VBANK },
-            hash: if enabled { hashes[&vbank_id] } else { 0 },
-        };
         unsafe {
             with_trace_ptr(trace, || {
                 crate::trace::bemu_btrace(
                     inst_id,
                     *hart_id as u64,
-                    slot(r0_enabled, r0_vbank),
-                    slot(r1_enabled, r1_vbank),
-                    slot(w0_enabled, w0_vbank),
+                    BTraceBank {
+                        vbank_id: w0_vbank,
+                        hash: status_hash,
+                    },
                     funct7 as u32,
                     &op_type,
                     pc,

src/nodes/bemu/native/ffi/mod.rs

@@ -1,4 +1,4 @@
-use bebop_bank_hash::{combine_bank_hash, BTraceBank, INVALID_VBANK};
+use bebop_bank_hash::{combine_bank_hash, BTraceBank};
 use bebop_bemu_profile::{BemuProfile, BemuProfileReport};
 use bebop_clint::Clint;
 use bebop_dtb::DtbBuilder;

src/nodes/bemu/src/trace/btrace.rs

@@ -40,12 +40,9 @@ pub(super) fn init(log_dir: &Path, enabled: bool) -> io::Result<BtraceState> {
     })
 }
 
-#[allow(clippy::too_many_arguments)]
 pub fn bemu_btrace(
     inst_id: u64,
     hart_id: u64,
-    r0: BTraceBank,
-    r1: BTraceBank,
     w0: BTraceBank,
     funct7: u32,
     op_type: &str,
@@ -57,8 +54,6 @@ pub fn bemu_btrace(
             BTraceSource::Bemu,
             inst_id,
             hart_id,
-            r0,
-            r1,
             w0,
             funct7,
             op_type,
@@ -67,10 +62,9 @@ pub fn bemu_btrace(
             Some(format!("{GOLDEN_RECORD_FILE}:{line_number}")),
         );
 
-        if let (Some(file), Ok(line)) = (trace.btrace.golden_record_file.as_mut(), record.to_ndjson()) {
-            file.write_all(line.as_bytes()).ok();
-            file.flush().ok();
-        }
+        let file = trace.btrace.golden_record_file.as_mut().expect("BTrace is enabled");
+        file.write_all(record.to_ndjson().unwrap().as_bytes()).unwrap();
+        file.flush().unwrap();
         observe(&record);
     });
 }

src/nodes/lib/bank-hash/src/comparator.rs

@@ -33,8 +33,8 @@ pub struct Comparison {
     pub result: CompareResult,
     pub hart_id: u64,
     pub inst_id: u64,
-    pub subject: Option<[BTraceBank; 3]>,
-    pub golden: Option<[BTraceBank; 3]>,
+    pub subject: Option<BTraceBank>,
+    pub golden: Option<BTraceBank>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub funct7: Option<u32>,
     #[serde(skip_serializing_if = "Option::is_none")]
@@ -43,7 +43,7 @@ pub struct Comparison {
 
 pub(crate) fn compare(key: CompareKey, rtl: Option<&BTraceRecord>, bemu: Option<&BTraceRecord>) -> Comparison {
     let result = match (rtl, bemu) {
-        (Some(rtl), Some(bemu)) if [rtl.r0, rtl.r1, rtl.w0] == [bemu.r0, bemu.r1, bemu.w0] => CompareResult::Pass,
+        (Some(rtl), Some(bemu)) if rtl.w0 == bemu.w0 => CompareResult::Pass,
         (Some(_), Some(_)) => CompareResult::Mismatch,
         (None, Some(_)) => CompareResult::MissingSubject,
         (Some(_), None) => CompareResult::MissingGolden,
@@ -55,8 +55,8 @@ pub(crate) fn compare(key: CompareKey, rtl: Option<&BTraceRecord>, bemu: Option<
         result,
         hart_id: key.hart_id,
         inst_id: key.inst_id,
-        subject: rtl.map(|record| [record.r0, record.r1, record.w0]),
-        golden: bemu.map(|record| [record.r0, record.r1, record.w0]),
+        subject: rtl.map(|record| record.w0),
+        golden: bemu.map(|record| record.w0),
         funct7: bemu
             .map(|record| record.funct7)
             .or_else(|| rtl.map(|record| record.funct7)),
@@ -67,10 +67,17 @@ pub(crate) fn compare(key: CompareKey, rtl: Option<&BTraceRecord>, bemu: Option<
 }
 
 fn records_by_key(records: impl IntoIterator<Item = BTraceRecord>) -> BTreeMap<CompareKey, BTraceRecord> {
-    records
-        .into_iter()
-        .map(|record| (CompareKey::from(&record), record))
-        .collect()
+    let mut keyed = BTreeMap::new();
+    for record in records {
+        let key = CompareKey::from(&record);
+        assert!(
+            keyed.insert(key, record).is_none(),
+            "duplicate BTrace record: hart={} inst={}",
+            key.hart_id,
+            key.inst_id
+        );
+    }
+    keyed
 }
 
 pub fn compare_offline(

src/nodes/lib/bank-hash/src/lib.rs

@@ -17,8 +17,6 @@ pub enum BTraceSource {
     Bemu,
 }
 
-pub const INVALID_VBANK: u32 = u32::MAX;
-
 #[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)]
 pub struct BTraceBank {
     pub vbank_id: u32,
@@ -32,19 +30,14 @@ pub enum BTraceTime {
     VerilatorTime(u64),
 }
 
-/// comparison record: <InstID, LogicalBankID, Hash>.
-///
-/// The remaining fields are diagnostic metadata and never participate in
-/// record alignment.
 #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
+#[serde(deny_unknown_fields)]
 pub struct BTraceRecord {
     #[serde(rename = "type")]
     pub record_type: String,
     pub source: BTraceSource,
     pub inst_id: u64,
     pub hart_id: u64,
-    pub r0: BTraceBank,
-    pub r1: BTraceBank,
     pub w0: BTraceBank,
     pub funct7: u32,
     pub op_type: String,
@@ -59,13 +52,10 @@ pub struct BTraceRecord {
 }
 
 impl BTraceRecord {
-    #[allow(clippy::too_many_arguments)]
     pub fn new(
         source: BTraceSource,
         inst_id: u64,
         hart_id: u64,
-        r0: BTraceBank,
-        r1: BTraceBank,
         w0: BTraceBank,
         funct7: u32,
         op_type: impl Into<String>,
@@ -83,8 +73,6 @@ impl BTraceRecord {
             source,
             inst_id,
             hart_id,
-            r0,
-            r1,
             w0,
             funct7,
             op_type: op_type.into(),

src/nodes/lib/bank-hash/tests/w0.rs

@@ -0,0 +1,85 @@
+use bebop_bank_hash::{compare_offline, BTraceBank, BTraceRecord, BTraceSource, BTraceTime, CompareResult};
+
+fn record(source: BTraceSource, hart_id: u64, inst_id: u64, vbank_id: u32, hash: u32) -> BTraceRecord {
+    BTraceRecord::new(
+        source,
+        inst_id,
+        hart_id,
+        BTraceBank { vbank_id, hash },
+        33,
+        "mvin",
+        BTraceTime::Cycle(1),
+        None,
+        None,
+    )
+}
+
+#[test]
+fn compares_only_the_matching_hart_and_instruction() {
+    let subject = [
+        record(BTraceSource::Rtl, 0, 7, 4, 11),
+        record(BTraceSource::Rtl, 1, 7, 5, 12),
+    ];
+    let golden = [
+        record(BTraceSource::Bemu, 1, 7, 5, 12),
+        record(BTraceSource::Bemu, 0, 7, 4, 11),
+    ];
+    let comparisons = compare_offline(subject, golden);
+    assert_eq!(comparisons.len(), 2);
+    assert!(comparisons
+        .iter()
+        .all(|comparison| comparison.result == CompareResult::Pass));
+    let diff = serde_json::to_value(&comparisons[0]).unwrap();
+    assert!(diff["subject"].is_object());
+    assert!(diff["golden"].is_object());
+}
+
+#[test]
+fn detects_wrong_hash_bank_and_unmatched_records() {
+    let subject = [
+        record(BTraceSource::Rtl, 0, 1, 4, 11),
+        record(BTraceSource::Rtl, 0, 2, 4, 11),
+        record(BTraceSource::Rtl, 0, 3, 4, 11),
+        record(BTraceSource::Rtl, 0, 4, 4, 11),
+    ];
+    let golden = [
+        record(BTraceSource::Bemu, 0, 1, 4, 12),
+        record(BTraceSource::Bemu, 0, 2, 5, 11),
+        record(BTraceSource::Bemu, 0, 5, 4, 11),
+    ];
+    let comparisons = compare_offline(subject, golden);
+    assert_eq!(
+        comparisons.iter().map(|item| item.result).collect::<Vec<_>>(),
+        [
+            CompareResult::Mismatch,
+            CompareResult::Mismatch,
+            CompareResult::MissingGolden,
+            CompareResult::MissingGolden,
+            CompareResult::MissingSubject,
+        ]
+    );
+}
+
+#[test]
+#[should_panic(expected = "duplicate BTrace record")]
+fn rejects_duplicate_records() {
+    compare_offline(
+        [
+            record(BTraceSource::Rtl, 0, 7, 4, 11),
+            record(BTraceSource::Rtl, 0, 7, 4, 12),
+        ],
+        [],
+    );
+}
+
+#[test]
+fn rejects_legacy_three_bank_record() {
+    let mut value = serde_json::to_value(record(BTraceSource::Rtl, 0, 7, 4, 11)).unwrap();
+    assert_eq!(value["hart_id"], 0);
+    assert_eq!(value["inst_id"], 7);
+    assert_eq!(value["w0"], serde_json::json!({"vbank_id": 4, "hash_u32": 11}));
+    assert!(value.get("r0").is_none());
+    assert!(value.get("r1").is_none());
+    value["r0"] = serde_json::json!({"vbank_id": 4294967295u32, "hash_u32": 0});
+    assert!(serde_json::from_value::<BTraceRecord>(value).is_err());
+}

src/nodes/lib/rtl-trace/src/banktrace.rs

@@ -22,13 +22,11 @@ pub fn init(log_dir: &Path, enabled: bool) -> io::Result<()> {
     Ok(())
 }
 
-pub fn btrace(inst_id: u64, hart_id: u64, r0: BTraceBank, r1: BTraceBank, w0: BTraceBank) {
+pub fn btrace(inst_id: u64, hart_id: u64, w0: BTraceBank) {
     let record = BTraceRecord::new(
         BTraceSource::Rtl,
         inst_id,
         hart_id,
-        r0,
-        r1,
         w0,
         0,
         "btrace".to_string(),

src/nodes/lib/rtl-trace/src/dpi.rs

@@ -9,7 +9,7 @@ pub(crate) fn force_link() {
     let _ = dpi_itrace as extern "C" fn(u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32);
     let _ = dpi_mtrace
         as extern "C" fn(u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32, u32);
-    let _ = dpi_btrace as extern "C" fn(u32, u32, u32, u32, u32, u32, u32, u32, u32, u32);
+    let _ = dpi_btrace as extern "C" fn(u32, u32, u32, u32, u32, u32);
     let _ = dpi_pmctrace as extern "C" fn(u32, u32, u32, u32);
     let _ = dpi_mem_pmctrace as extern "C" fn(u32, u32, u32, u32);
     let _ = dpi_ctrace as extern "C" fn(u32, u32, u32, u32, u32, u32, u32, u32);
@@ -100,25 +100,13 @@ pub extern "C" fn dpi_btrace(
     inst_id_hi: u32,
     hart_id_lo: u32,
     hart_id_hi: u32,
-    r0_vbank: u32,
-    r0_hash: u32,
-    r1_vbank: u32,
-    r1_hash: u32,
     w0_vbank: u32,
     w0_hash: u32,
 ) {
     state::record_btrace_callback();
     crate::banktrace::btrace(
         u64_from_words(inst_id_lo, inst_id_hi),
         u64_from_words(hart_id_lo, hart_id_hi),
-        bebop_bank_hash::BTraceBank {
-            vbank_id: r0_vbank,
-            hash: r0_hash,
-        },
-        bebop_bank_hash::BTraceBank {
-            vbank_id: r1_vbank,
-            hash: r1_hash,
-        },
         bebop_bank_hash::BTraceBank {
             vbank_id: w0_vbank,
             hash: w0_hash,