Skip to content

feat: add design version info at runtime #508

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.

Sign up for GitHub

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

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

feat: add design version info at runtime #508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ RTL_DIR = $(BUILD_DIR)/rtl
RTL_SUFFIX ?= sv
SIM_TOP_V = $(RTL_DIR)/$(SIM_TOP).$(RTL_SUFFIX)

# get design version info
VERSION_COMMIT_ID := $(shell git -C $(DESIGN_DIR) rev-parse --short HEAD)
VERSION_IS_DIRTY := $(shell git -C $(DESIGN_DIR) diff --quiet HEAD || echo "-dirty")

@poemonsense poemonsense Nov 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. Do we need to check whether git exists?

  2. I think we can simply pass them as a C macro.

SIM_CXXFLAGS += -DDUT_VERSION_STRING=\\\"${VERSION_COMMIT_ID}${VERSION_IS_DIRTY}\\\"
  1. Maybe we can also add the DIFFTEST_VERSION_STRING.

@cebarobot cebarobot Nov 16, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

  1. I think we can simply pass them as a C macro.
SIM_CXXFLAGS += -DDUT_VERSION_STRING=\\\"${VERSION_COMMIT_ID}${VERSION_IS_DIRTY}\\\"

This won't work, for two reasons.

  • Verilator writes all FLAGS into VSimTop.mk, which won't be update as long as verilator is not triggered again.
  • The original source code file common.cpp (and its timestamp) is never changed. In that way, the compiling system make will think the compiled result common.o is always up-to-date and no need to be re-compiled. Hence the commit id will always keep the old one when the whole project is compiled for the first time.

@cebarobot cebarobot Nov 16, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Check git is a good proposal. I'm considering to genenrate the commit info string in the design repo and pass it to difftest.

@cebarobot cebarobot Nov 16, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Difftest commit info is also valuable, but I think it's better to:

  • add all submodules commit info
difftest: asdfqwe
yunsuan: qwerasd
...
  • or, record detail dirty info by git status --porcelain, since the main repo commit has included the submodule commit info.
 M difftest

Which one do you think is better?

@poemonsense poemonsense Nov 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Verilator writes all FLAGS into VSimTop.mk, which won't be update as long as verilator is not triggered again.

This is what we want as this dut version should be the same as the generated verilog, not the git repo.

Consider this case: make emu is generated with version A. We checkout it to version B. If we enforce a make emu again, the timestamp will be changed to version B. This is not what we want.

The DUT version should be exactly matching the verilog code, which is passed at verilator build time instead of the current git commit.

@poemonsense poemonsense Nov 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • add all submodules commit info

difftest is a general co-sim framework for RISC-V processors, not only for XiangShan.

The reason we use git here is for the general-purpose usage. If XiangShan, or other DUTs, want a more complicated version string, they should pass the string by command line DUT_VERSION_STRING=XXX.


# generate difftest files for non-chisel design.
.DEFAULT_GOAL := difftest_verilog
ifeq ($(MFC), 1)
Expand Down Expand Up @@ -227,6 +231,12 @@ ifeq ($(CXX_NO_WARNING),1)
SIM_CXXFLAGS += -Werror
endif

# Generate Version Info File (Always)

@poemonsense poemonsense Nov 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is no need to generate this header. See below comments for common.cpp

VERSION_HEADER = $(GEN_CSRC_DIR)/version.h
.PHONY: $(VERSION_HEADER)
$(VERSION_HEADER):
@echo "#define STR_COMMIT_ID \"$(VERSION_COMMIT_ID)\"\n#define STR_IS_DIRTY \"$(VERSION_IS_DIRTY)\"" > $@

include verilator.mk
include vcs.mk
include palladium.mk
Expand Down
3 changes: 3 additions & 0 deletions src/test/csrc/common/common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
***************************************************************************************/

#include "common.h"
#include "version.h"
#include <locale.h>
#include <signal.h>

Expand Down Expand Up @@ -70,6 +71,8 @@ void common_init_without_assertion(const char *program_name) {
elf_name = elf_name ? elf_name + 1 : program_name;
Info("%s compiled at %s, %s\n", elf_name, __DATE__, __TIME__);

@poemonsense poemonsense Nov 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. There's no need to add one more line for the version string. We can simply add them to the elf_name.
Info("%s(" DUT_VERSION_STRING ", " DIFFTEST_VERSION_STRING ") compiled at " __DATE__ ", __TIME__ " \n", elf_name);


Info("Source Code Version: %s%s\n", STR_COMMIT_ID, STR_IS_DIRTY);

// set buffer for stderr
setbuf(stderr, mybuf);

Expand Down
2 changes: 1 addition & 1 deletion vcs.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ VCS_FLAGS += $(EXTRA)

VCS_VSRC_DIR = $(abspath ./src/test/vsrc/vcs)
VCS_VFILES = $(SIM_VSRC) $(shell find $(VCS_VSRC_DIR) -name "*.v")
$(VCS_TARGET): $(SIM_TOP_V) $(VCS_CXXFILES) $(VCS_VFILES)
$(VCS_TARGET): $(SIM_TOP_V) $(VCS_CXXFILES) $(VCS_VFILES) $(VERSION_HEADER)

@poemonsense poemonsense Nov 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we are passing versions with C macros, there's no need to creating the headers here.

$(VCS) $(VCS_FLAGS) $(SIM_TOP_V) $(VCS_CXXFILES) $(VCS_VFILES)
ifeq ($(VCS),verilator)
$(MAKE) -s -C $(VCS_BUILD_DIR) -f V$(VCS_TOP).mk
Expand Down
2 changes: 1 addition & 1 deletion verilator.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ else
PGO_LDFLAGS="'"$(PGO_LDFLAGS)"'"'
endif

$(EMU): $(EMU_MK) $(EMU_DEPS) $(EMU_HEADERS)
$(EMU): $(EMU_MK) $(EMU_DEPS) $(EMU_HEADERS) $(VERSION_HEADER)
@echo -e "\n[c++] Compiling C++ files..." >> $(TIMELOG)
@date -R | tee -a $(TIMELOG)
ifdef PGO_WORKLOAD
Expand Down