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

Questa: start simulation from PWD, not PWD/sim_build #1063

Merged
merged 3 commits into from
Feb 17, 2020

Conversation

TC01
Copy link
Contributor

@TC01 TC01 commented Aug 8, 2019

The Icarus and Cadence (ius) simulator targets run the simulation relative to ${PWD}; i.e. the location of the Makefile. The Questa one, though, seems to run everything out of the ${PWD}/sim_build directory.

This commit changes the questa Makefile to run from ${PWD}, while keeping the "work" library and the run script in sim_build, for better compatibility with the other simulators.

In particular, I just tried to port a project from Cadence to Questa and found that, because my Makefile didn't explicitly refer to ${PWD} but instead just provided source paths relative to the current working directory, the simulation would not start. Hence this change.

Hopefully this does not break anything: it would be great if people with more mature Questa projects could test!

I should say: it's possible that this isn't desired behavior. In which case, maybe the other simulators should be changed instead for consistency? (This all also may be immaterial if the makefile approach to cocotb is replaced with the purer-Python cocotb-test approach, but assuming we keep some kind of compatibility layer with the Makefiles, it would probably still be good to fix this).

@TC01
Copy link
Contributor Author

TC01 commented Aug 8, 2019

For what it's worth, here is a very simple example project where simulation successfully starts with Cadence and Icarus but fails with Questa for this issue.

@themperek
Copy link
Contributor

themperek commented Aug 15, 2019

Hard to say what should be a desirable way. VCS and aldec makefile seem also different.

In cocotb-test I decided to run all simulators form sim_build.

One alternative (maybe in any case) would be to make an absolute path for sources in Makefile (maybe here: https://github.com/cocotb/cocotb/blob/master/cocotb/share/makefiles/Makefile.sim) before giving this to the simulator (I do this in cocotb-test). In makefile can call python to do this or $(abspath ..) for every source file.

In any case, if we change should change everywhere the same way. Touching those Makefiles is always "controversial".

@TC01
Copy link
Contributor Author

TC01 commented Aug 15, 2019

One alternative (maybe in any case) would be to make an absolute path for sources in Makefile (maybe here: https://github.com/cocotb/cocotb/blob/master/cocotb/share/makefiles/Makefile.sim) before giving this to the simulator (I do this in cocotb-test).

I like this idea, it seems much more elegant than just moving the location the simulator starts from around. (I briefly wondered if this could be done by passing an argument to vsim/vlog but didn't think to try changing it in Makefile.sim).

For the makefile approach, perhaps we could have some new parameter (SRC_DIR or RTL_SRC_DIR or similar) that defaults to the current working directory but could be changed at the users discretion?

I can try to put that together and submit a new PR.

Separately, I'm still wondering whether or not it would be good to be consistent about where the simulators start from, but as you say it'd be better to fix that across all the simulators rather than one at a time. (I only have access to questa, ius, and icarus at the moment, which is why I only looked at those).

@themperek
Copy link
Contributor

@TC01 in cocotb-test I did different/simple compatibility/adoption (https://github.com/themperek/cocotb-test/blob/master/cocotb_test/Makefile.inc) so we do not have to maintain 2 systems.

We need @imphil to help on this.

@imphil
Copy link
Member

imphil commented Aug 26, 2019

Thanks for this PR and your quest for consistency @TC01! I'd say the current behavior of how relative paths in VERILOG_SOURCES/VHDL_SOURCES is somewhat "undefined behavor"; all our examples and our documentation use absolute paths.

To push this forward,

  1. We should define the desired behavior: if we encounter a relative path in VERILOG_SOURCES or VHDL_SOURCES, what is it relative to?
  2. We need to document this behavior.
  3. We should make sure all simulators behave the same.

My thinking on this:

  1. The only sane way for relative paths to behave is relative to the file they are mentioned in. That means relative to the Makefile, which is the behavior we currently have for icarus and ius.
  2. Can you add a commit updating the documentation in documentation/source/building.rst regarding these two variables?
  3. Your approach is one option to ensure that relative source paths are interpreted relative to the Makefile. Another would be to transform all paths to absolute ones. I'm fine with both approaches.

@TC01
Copy link
Contributor Author

TC01 commented Aug 26, 2019

Sure! I'll push a new commit here to update the documentation.

Copy link
Member

@imphil imphil left a comment

Choose a reason for hiding this comment

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

Thanks for your update @TC01! I have left a couple comments inline.

cocotb/share/makefiles/simulators/Makefile.questa Outdated Show resolved Hide resolved
cocotb/share/makefiles/simulators/Makefile.questa Outdated Show resolved Hide resolved
documentation/source/building.rst Outdated Show resolved Hide resolved
@TC01
Copy link
Contributor Author

TC01 commented Sep 17, 2019

I also just noticed that these changes moved sim.log from $PWD/sim_build/sim.log to $PWD/sim.log. I've just pushed another commit fixing that.

Let me know how I should proceed here: if this looks okay as is or if we want to take a different approach in making the simulators consistent.

@cmarqu
Copy link
Contributor

cmarqu commented Oct 23, 2019

Related: #838

@TC01
Copy link
Contributor Author

TC01 commented Nov 15, 2019

PIng @imphil?

The Icarus and Cadence (ius) simulator targets run the simulation
relative to PWD; i.e. the location of the Makefile. The Questa one,
though, seems to run everything out of the sim_build directory.

This commit changes the questa Makefile to run from ${PWD}, while
keeping the "work" library and the run script in sim_build, for better
compatibility with the other simulators.

In particular, I just tried to port a project from Cadence to Questa
and found that, because my Makefile didn't say ${PWD} but instead
just provided source paths relative to the current working directory,
the simulation would not start. Hence this change.

Hopefully this does not break anything: it would be great if people
with Questa projects could test!
If the paths are relative, then they will be interpreted as relative
to the Makefile location (i.e. the file that they are defined in).

This may not be true for all simulators at the moment, in which case
that should be fixed.
@imphil
Copy link
Member

imphil commented Feb 16, 2020

@TC01 sorry for the significant delay. I've rebased this PR on top of master, and will merge it once CI passes.

@imphil imphil merged commit 6bd77d0 into cocotb:master Feb 17, 2020
themperek added a commit to themperek/cocotb that referenced this pull request Apr 7, 2020
themperek added a commit to themperek/cocotb that referenced this pull request Apr 7, 2020
themperek added a commit to themperek/cocotb that referenced this pull request Apr 7, 2020
themperek added a commit to themperek/cocotb that referenced this pull request Apr 7, 2020
themperek added a commit that referenced this pull request Apr 7, 2020
themperek added a commit that referenced this pull request Apr 7, 2020
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.

4 participants