Fix handling of RNG seed #1369
Merged
Fix handling of RNG seed #1369
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gatecat
reviewed
Sep 18, 2024
common/kernel/property.h
Outdated
| @@ -42,7 +42,7 @@ struct Property | |||
| }; | |||
|
|
|||
| Property(); | |||
| Property(int64_t intval, int width = 32); | |||
| Property(int64_t intval, int width = 64); | |||
There was a problem hiding this comment.
I'm concerned this might end up having unwanted consequences elsewhere - perhaps you could just instantiate it 64 bits wide in the one place where seed is being saved?
jthornblad
force-pushed
the
fix-seed-usage
branch
from
364eef2
to
f4eb79f
Compare
|
Does this look ok? |
gatecat
reviewed
Sep 18, 2024
common/kernel/command.cc
Outdated
| @@ -565,7 +565,8 @@ void CommandHandler::setupContext(Context *ctx) | |||
|
|
|||
| ctx->settings[ctx->id("arch.name")] = std::string(ctx->archId().c_str(ctx)); | |||
| ctx->settings[ctx->id("arch.type")] = std::string(ctx->archArgsToId(ctx->archArgs()).c_str(ctx)); | |||
| ctx->settings[ctx->id("seed")] = ctx->rngstate; | |||
| Property seed_property(ctx->rngstate, 64); | |||
| ctx->settings[ctx->id("seed")] = seed_property; | |||
There was a problem hiding this comment.
what about ctx->settings[ctx->id("seed")] = Property(ctx->rngstate, 64);? that's closer to the style we tend to use.
* Fix truncation of output seed value from 64 bits to 32 bits (int instead of uint64) when written to json file. * Fix input seed value conversion when --seed option is used. * Remove input seed value scrambling (use of rngseed()) when --seed or --randomize-seed option is used since the output seed value will be the scrambled value and not the seed that was actually supplied or generated.
jthornblad
force-pushed
the
fix-seed-usage
branch
from
f4eb79f
to
2541ecc
Compare
|
Good suggestion, done. |
|
Thanks! |
|
I see that the CI is failing for this patch. I'm not quite sure what the problem is, do you have any idea? |
|
Looks like one of the tests is timing out (probably a latent instability) |
rowanG077
added a commit
to rowanG077/nextpnr-tests
that referenced
this pull request
Sep 19, 2024
|
Input seed handling has changed which means some fixed seeds in the test suite are causing issues. YosysHQ/nextpnr-tests#12 should fix it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
There are a couple of problems with the handling of the RNG seed.
First, the seed value used for the deterministic RNG is being truncated from 64 bits to 32 (int instead of uint64) when written to the header of the output JSON file.
When using the --seed option on the command line, the argument needs to be a signed value.
If using the --seed or --randomize-seed option, it is not the originally supplied or randomized seed value that is written to the output JSON file, but a value that has been run through the rngseed() function, which permutes the value before it is stored and then used for the output JSON file.
This patch fixes these problems.
Regards
Jonas