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

feat(query): support query for difftest-dpic data #557

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

klin02
Copy link
Member

@klin02 klin02 commented Jan 31, 2025

No description provided.

@klin02
Copy link
Member Author

klin02 commented Jan 31, 2025

It also works in Palladium with some viarable set as follows:
export C_INCLUDE_PATH=/path/to/sqlite3/include
export CPLUS_INCLUDE_PATH=/path/to/sqlite3/include
User need to install sqlite3 manually to /path/to/sqlite3.

And in Palladium, after we enable query feature, simulation speed will decrease from 450KHz to 280 KHz (20s -> 30s).

@klin02 klin02 requested a review from poemonsense February 1, 2025 04:47
Copy link
Member

@poemonsense poemonsense left a comment

Choose a reason for hiding this comment

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

  1. what is this query for? what's the relationship between DPIC and query?

  2. should we always generate the query headers?

@klin02
Copy link
Member Author

klin02 commented Feb 1, 2025

  1. what is this query for? what's the relationship between DPIC and query?
  2. should we always generate the query headers?
  1. The query function is used to record the data of the DPIC transmission, thus helping to find out optimization strategy of transmission. For example, we can calculate compress ratio of each buffer when incremental transmitting. Also, record of DPIC data helps locating bug in some debugging case.
  2. We always generate query headers to avoid generating hardware repeatly for some debugging case, or in some case we will firstly compare performance without Query, then exploring possible reason through Query. This feature will only be enabled when defined CONFIG_DIFFTEST_QUERY.

Copy link
Member

@poemonsense poemonsense left a comment

Choose a reason for hiding this comment

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

I don't know much on the functionality of the query but the implementation can be improved.

Functions should be defined in src and they will read the predefiend data structures generated by Scala. This is a better way to coordinate between different tools -- they follow some predefined interfaces, especially the C++ structs/classes.

Also, the query C++ functions look similar to ChiselDB. Should we provide some standard DB APIs in DiffTest?

These suggestions are only for future improvements. I don't mind this PR being merged first to help XS development.

| snprintf (query_path, 128, "%s/build/%s", getenv("NOOP_HOME"), "difftest_query.db");
| // remove exist file
| FILE* fp = fopen(query_path, "r");
| if (fp) { fclose(fp); remove(query_path);}
Copy link
Member

Choose a reason for hiding this comment

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

I think these common C++ code should be put in src/test/csrc instead of the Scala code. If we can define the clear data structures for the query, then only the data structures should be generated by Scala. Similar to how DiffState is generated and used, and how the coverage metrics are defined.

Also, when put in src, these C++ code can be formatted. The current form in Scala does not look good.

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.

2 participants