Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[difftest] Add offline difftest and refactor libspike interfaces #603

Merged
merged 19 commits into from
Jun 17, 2024

Conversation

Clo91eaf
Copy link
Contributor

No description provided.

@sequencer
Copy link
Member

cc @Avimitin please add it to build system and run all elf to check the correctness of this program.

@Avimitin Avimitin force-pushed the offline-difftest branch 5 times, most recently from 7e15390 to 91b98b5 Compare May 27, 2024 03:52
@Clo91eaf Clo91eaf force-pushed the offline-difftest branch 2 times, most recently from eaf9627 to b03f146 Compare May 30, 2024 18:55
@Clo91eaf Clo91eaf force-pushed the offline-difftest branch from 3231db7 to ca2a4a9 Compare June 7, 2024 07:17
@Avimitin Avimitin force-pushed the offline-difftest branch 2 times, most recently from b32b2d2 to 34310a0 Compare June 8, 2024 05:25
@Clo91eaf Clo91eaf force-pushed the offline-difftest branch 3 times, most recently from 1a766cf to f552046 Compare June 10, 2024 07:42
@Clo91eaf Clo91eaf force-pushed the offline-difftest branch 4 times, most recently from 5871d97 to 87c601a Compare June 11, 2024 03:26
@Clo91eaf Clo91eaf requested a review from sequencer June 11, 2024 03:26
sequencer
sequencer previously approved these changes Jun 11, 2024
Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really appreciate your work! Please add CI for offline difftest, we will let online and offline run at the same time for a while, and get rid of runtime difftest.

@@ -22,10 +22,10 @@ spike_state_t* proc_get_state(spike_processor_t* proc);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add documentation to header files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And please think a question:
if we are going to use sail/qemu to replace spike in the future, how do we decouple this?
maybe riscv_architecture_events.h

@@ -0,0 +1,11 @@
## Build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add more detail readme on the difftest, including the rational.md.

  • the architecture of the difftest design.
  • the public interface and usage of the difftest, aka the schema of all events.
    Please do it in the following PR.

let event = self.dut.step()?;

match &*event.event {
"peekTL" => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the following PR, please rename it to "memoryWrite", in the next architecture change, we will refactor T1 to AXI.

let cycle = event.parameter.cycle.unwrap();
self.peek_issue(IssueEvent { idx, cycle }).unwrap();
}
"lsuEnq" => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be refactor to RTL w/ tagging.

let cycle = event.parameter.cycle.unwrap();
self.update_lsu_idx(LsuEnqEvent { enq, cycle }).unwrap();
}
"vrfWriteFromLsu" => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be merged w/ vrfWriteFromLane

})
.unwrap();
}
"issue" => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this commit?

})
.unwrap();
}
"inst" => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this is issue

difftest/t1-simulator/src/difftest.rs Outdated Show resolved Hide resolved
difftest/t1-simulator/src/main.rs Outdated Show resolved Hide resolved
difftest/t1-simulator/src/main.rs Outdated Show resolved Hide resolved
@Avimitin Avimitin force-pushed the offline-difftest branch 3 times, most recently from f63e4b1 to 270d56d Compare June 11, 2024 19:54
@Clo91eaf Clo91eaf force-pushed the offline-difftest branch 4 times, most recently from 03c17ef to 61577cd Compare June 14, 2024 17:23
@Avimitin Avimitin force-pushed the offline-difftest branch 3 times, most recently from ff0016a to 6aa7b46 Compare June 17, 2024 04:02
sequencer
sequencer previously approved these changes Jun 17, 2024
Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally!

Avimitin added 2 commits June 17, 2024 17:54
- Use nix derivation to cache and capture emulator output
- Use built-in S3 nix cache to replace GitHub Artifacts
- Produce GitHub step summary in Scala script

Signed-off-by: Avimitin <[email protected]>
@sequencer sequencer merged commit 6247e55 into master Jun 17, 2024
@sequencer sequencer deleted the offline-difftest branch June 17, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants