[RFC] Separate project make targets/variables from Makefile #496
Conversation
| include $(base_dir)/generators/ariane/ariane.mk | ||
| include $(base_dir)/generators/tracegen/tracegen.mk | ||
| include $(base_dir)/tools/dromajo/dromajo.mk | ||
|
|
There was a problem hiding this comment.
I get that making this change against the dromajo PR is done to show why this splitting is useful, but I'm still apprehensive about making so many changes in a chipyard feature branch. If its not too much work, and people like these changes, can we start with a PR into the dev branch that does this restructuring for just ariane and tracegen, and merge in dromajo support later?
There was a problem hiding this comment.
Sure, we can split this up... I just want to show how this might be done nicely with something that needs it more (i.e. Dromajo). If people agree on this approach then we can migrate it to dev.
There was a problem hiding this comment.
so what happens when these projects settings conflict?
There was a problem hiding this comment.
this is strictly better than what we had before though.
There was a problem hiding this comment.
Currently, everything is additive (see the += for all the flags in their respective mks). However, I haven't put much thought into how we deal with conflicts (for ex. when +drj_bin= is defined twice).
There was a problem hiding this comment.
I know that the simulator will probably error... but maybe having some sort of check in common.mk would be nice? Maybe warn if full words match between mks for the SIM_FLAGS variable (i.e. so that +arg +arg is caught)?
There was a problem hiding this comment.
I agree with Jerry that we should do a very small feature addition here and mostly just do the refactor (perhaps only do the refactor of tracegen), to iron out the changes.
We also need buy-in from all devs because with the number of make targets changing there is a high likelihood that you are breaking someones workflow.
common.mk
Outdated
|
|
||
| $(output_dir)/%.out: $(output_dir)/% $(sim) | ||
| (set -o pipefail && $(sim) $(PERMISSIVE_ON) +dramsim +max-cycles=$(timeout_cycles) $(VERBOSE_FLAGS) $(PERMISSIVE_OFF) $< </dev/null 2> >(spike-dasm > $@) | tee $<.log) | ||
| (set -o pipefail && $(sim) $(PERMISSIVE_ON) +dramsim +max-cycles=$(timeout_cycles) $(VERBOSE_FLAGS) $(SIM_FLAGS) $(PROJECT_SIM_FLAGS) $(PERMISSIVE_OFF) $< </dev/null 2> >(spike-dasm > $@) | tee $<.log) |
There was a problem hiding this comment.
I think now that we have SIM_FLAGS we should move dramsim and max-cycles in there.
There was a problem hiding this comment.
I don't know what dramsim does but I think we should keep these two separate.
In many cases, I have to add extra flags to the simulation with SIM_FLAGS so having these both move there will make it so that adding new variables also requires adding the +dramsim +max-cycles flags as well as +abe-flag=1.
A compromise that we can do is have the SIM_FLAGS use override and just add the +dramsim +max-cycles always? Thoughts?
There was a problem hiding this comment.
Yeah override is the solution for this.
override SIM_FLAGS += +dramsim +max-cycles
There was a problem hiding this comment.
Yup Ill change it to that then
tools/dromajo/dromajo.mk
Outdated
| PROJECT_SIM_FLAGS += $(DROMAJO_FLAGS) | ||
|
|
||
| # extra vcs compile flags | ||
| PROJECT_VCS_FLAGS += -CC "-I$(DROMAJO_DIR)" $(DROMAJO_LIB) |
There was a problem hiding this comment.
There is a nice refactor here where you have PROJECT_SIM_CC_FLAGS set to -I$(DROMAJO_DIR) and verilator.mk and vcs.mk use that same variable.
There was a problem hiding this comment.
Ok how does this look now?
There was a problem hiding this comment.
Looks good aside from the question I had about multiple EXTRA_SIM_CC_FLAGS.
| @@ -50,7 +50,8 @@ VCS_CC_OPTS = \ | |||
| -CC "-I$(dramsim_dir)" \ | |||
| -CC "-std=c++11" \ | |||
| $(dramsim_lib) \ | |||
| $(RISCV)/lib/libfesvr.a | |||
| $(RISCV)/lib/libfesvr.a \ | |||
| -CC "$(EXTRA_SIM_CC_FLAGS)" | |||
There was a problem hiding this comment.
Does this work if there are more than 1 extra flags?
There was a problem hiding this comment.
Yup. If there are extra flags in the EXTRA_SIM_CC_FLAGS this works.
There was a problem hiding this comment.
Are you sure that vcs does the right thing w.r.t. -CC "-Iblah -Iblah2"
There was a problem hiding this comment.
I just tried the -CC "-I$(VCS_HOME)/include -I$(RISCV)/include -I$(dramsim_dir) -std=c++11" on one line and that worked.
|
Closing since this is split up into #499 and later into the |
Related issue:
Type of change: other enhancement
Impact: other
Release Notes
This is the first attempt to have sub-project Makefile changes separated from the main makefile. It allows
mkfragments in subprojects to inject variables/targets into the various parts of the makefile.Note: This is not targeting
devso this will be merged in at a later point.