Skip to content

Config file bug cleanup#370

Merged
JosephMoore25 merged 15 commits intodevfrom
config-bug-fixes
Feb 9, 2024
Merged

Config file bug cleanup#370
JosephMoore25 merged 15 commits intodevfrom
config-bug-fixes

Conversation

@JosephMoore25
Copy link
Copy Markdown
Contributor

@JosephMoore25 JosephMoore25 commented Jan 16, 2024

This PR aims to squash the bugs being investigated that appear through certain parameter values in the config file.

The current known issues are:

[FIXED] Vector-Length being greater than Load-Bandwidth or Store-Bandwidth causes progression to halt with no error. A simple check is needed in ModelConfig.cc to ensure this is not the case, and to error out if it is the case.

[FIXED] Fetch-Block-Size being greater than 128 (despite being a valid value) causes a large memory leak. This was due to the precision of bufferedBytes_ and bufferOffset being 8 bit, as was the bytesAvailable parameter of the function predecode. This led to an infinite loop of instruction fetches.

[TODO, not in this PR] Setting GeneralPurpose-Count or FloatingPoint/SVE-Count to a small value above architectural register values in OoO/MicroOp mode causes indefinite stalls due to not being able to rename all destination registers. This is to be handled in a different PR due to this not being a trivial fix.

[DONE] Investigate further parameter values to check for unexpected behaviour. Nothing is noticably wrong at this time.

@JosephMoore25 JosephMoore25 added the bug Something isn't working label Jan 16, 2024
@JosephMoore25 JosephMoore25 self-assigned this Jan 16, 2024
@FinnWilkinson FinnWilkinson linked an issue Jan 16, 2024 that may be closed by this pull request
@FinnWilkinson FinnWilkinson added the 0.9.6 Part of SimEng Release 0.9.6 label Jan 16, 2024
@FinnWilkinson FinnWilkinson marked this pull request as draft January 16, 2024 20:14
@JosephMoore25 JosephMoore25 marked this pull request as ready for review January 19, 2024 12:19
Copy link
Copy Markdown
Contributor

@dANW34V3R dANW34V3R left a comment

Choose a reason for hiding this comment

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

Minor comments, but concepts look good

Comment thread src/lib/config/ModelConfig.cc Outdated
Comment thread src/lib/config/ModelConfig.cc Outdated
@dANW34V3R
Copy link
Copy Markdown
Contributor

It could be worth adding tests for the maximum and minimum bound of each config option e.g. just run the default program for these options and make sure it runs to completion. This would be far from covering all potential issues as all options are dependent but would omit basic issues in the future. This would probably take a non-trivial amount of time so something that could be added to the CI pipeline. Thoughts @jj16791 @FinnWilkinson @ABenC377 @JosephMoore25 ?

@JosephMoore25
Copy link
Copy Markdown
Contributor Author

@dANW34V3R Ideally this would be tested. Problem is for ~23 parameters that would be 46 tests. In the case that one breaks, bounds would need to be set on the program. A timeout would be needed in the case of broken progression, and I guess memory leaks should eventually crash - these things may be slightly easier to monitor manually.

It may be sufficient to do this once before each release - I can provide an internal tool to run these tests automatically.

Our options are:

  1. Add tests to the pipeline. Pipeline will become noticably longer, but each push we confirm the bounds of all individual parameters (but can not guarantee dependent parameter combinations still)

  2. Use an internal tool to run this test semi-automated, to be run before releases

  3. Manually test bounds when changing code that may affect these (especially precision of variables)

We should take a vote/discussion on this, but the outcome is unlikely to change this PR. @FinnWilkinson @jj16791 @dANW34V3R

I'm under the impression that 2 should be sufficient and 3 should always be done anyway. These limits are often not touched through normal use of SimEng, but instead when designing extreme CPUs, hence not running into an issue until now.

dANW34V3R
dANW34V3R previously approved these changes Jan 19, 2024
@FinnWilkinson
Copy link
Copy Markdown
Contributor

It could be worth adding tests for the maximum and minimum bound of each config option e.g. just run the default program for these options and make sure it runs to completion. This would be far from covering all potential issues as all options are dependent but would omit basic issues in the future. This would probably take a non-trivial amount of time so something that could be added to the CI pipeline. Thoughts @jj16791 @FinnWilkinson @ABenC377 @JosephMoore25 ?

This would be nice to do yes, but I think we would need to use some other benchmark other than the default one to ensure all potential issues are discovered. For example, without running a code using SVE instructions, one of the bugs fixed by this PR wouldn't have been discovered

@FinnWilkinson
Copy link
Copy Markdown
Contributor

@dANW34V3R Ideally this would be tested. Problem is for ~23 parameters that would be 46 tests. In the case that one breaks, bounds would need to be set on the program. A timeout would be needed in the case of broken progression, and I guess memory leaks should eventually crash - these things may be slightly easier to monitor manually.

It may be sufficient to do this once before each release - I can provide an internal tool to run these tests automatically.

Our options are:

  1. Add tests to the pipeline. Pipeline will become noticably longer, but each push we confirm the bounds of all individual parameters (but can not guarantee dependent parameter combinations still)
  2. Use an internal tool to run this test semi-automated, to be run before releases
  3. Manually test bounds when changing code that may affect these (especially precision of variables)

We should take a vote/discussion on this, but the outcome is unlikely to change this PR. @FinnWilkinson @jj16791 @dANW34V3R

I'm under the impression that 2 should be sufficient and 3 should always be done anyway. These limits are often not touched through normal use of SimEng, but instead when designing extreme CPUs, hence not running into an issue until now.

We run a semi-automated script on the project before release to check that all exisiting benchmarks still functionally work, so doing this for something else wouldn't be too bad (in my opinion). However, upkeeping such a tool would require a bit more work given it is subject to change much more often than the list of supported benchmarks.

An alternative would be to implement unit / integration tests to ensure all the bounds are functioning correctly - this option would need to be a PR in itself though

Comment thread src/lib/config/ModelConfig.cc Outdated
Comment thread src/lib/config/ModelConfig.cc Outdated
@dANW34V3R
Copy link
Copy Markdown
Contributor

We run a semi-automated script on the project before release to check that all exisiting benchmarks still functionally work, so doing this for something else wouldn't be too bad (in my opinion). However, upkeeping such a tool would require a bit more work given it is subject to change much more often than the list of supported benchmarks.

Check on release seems like a possible solution to get the tests run but not unnecessarily over checking. I can see a way where it could be automated by checking the values set by functions like setValueBounds and setValueSet which would mean no manual upkeep.

An alternative would be to implement unit / integration tests to ensure all the bounds are functioning correctly - this option would need to be a PR in itself though

This would be option 1 I guess as all GTests are run in the CI pipeline on every push to an open PR. Check on release seems like a better option if these tests take some time to run

FinnWilkinson
FinnWilkinson previously approved these changes Jan 24, 2024
Merged dev updates into this branch (attempt 2, oops)
Comment thread src/lib/config/ModelConfig.cc Outdated
Comment thread src/lib/config/ModelConfig.cc Outdated
ABenC377
ABenC377 previously approved these changes Feb 8, 2024
Comment thread src/lib/config/ModelConfig.cc Outdated
@JosephMoore25 JosephMoore25 merged commit 8af924c into dev Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.9.6 Part of SimEng Release 0.9.6 bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Verify Config file parameters

5 participants