[PATCH risu] use time() as random seed and introduce --randseed option

[PATCH risu] use time() as random seed and introduce --randseed option

[Jun Sun]

[-- Attachment #1.1: Type: text/plain, Size: 230 bytes --]

By default, risu currently does not generate random instruction sequences
because it uses 0 as the random seed.  This patch uses time() as random
seed and also introduces --randomseed option for deterministic sequence
generation.

[-- Attachment #1.2: Type: text/html, Size: 283 bytes --]

[-- Attachment #2: 0008-add-randseed-option-and-use-time-as-default-seed.patch --]
[-- Type: application/octet-stream, Size: 5306 bytes --]


Signed-off-by: Jun Sun <jsun@junsun.net>
---
 risugen                |  4 ++++
 risugen_arm.pm         | 11 ++++++++---
 risugen_loongarch64.pm |  9 +++++++--
 risugen_m68k.pm        |  9 +++++++--
 risugen_ppc64.pm       | 10 +++++++---
 5 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/risugen b/risugen
index fa94a39..360112f 100755
--- a/risugen
+++ b/risugen
@@ -293,6 +293,7 @@ and outputfile is the generated raw binary file.
 
 Valid options:
     --numinsns n : generate n instructions (default is 10000)
+    --randseed n : use n as random generator seed (default uses time())
     --fpscr n    : set initial FPSCR (arm) or FPCR (aarch64) value (default is 0)
     --condprob p : [ARM only] make instructions conditional with probability p
                    (default is 0, ie all instructions are always executed)
@@ -317,6 +318,7 @@ EOT
 sub main()
 {
     my $numinsns = 10000;
+    my $randseed=-1;
     my $condprob = 0;
     my $fpscr = 0;
     my $fp_enabled = 1;
@@ -326,6 +328,7 @@ sub main()
 
     GetOptions( "help" => sub { usage(); exit(0); },
                 "numinsns=i" => \$numinsns,
+                "randseed=i" => \$randseed,
                 "fpscr=o" => \$fpscr,
                 "group=s" => \@groups,
                 "pattern=s" => \@pattern_re,
@@ -365,6 +368,7 @@ sub main()
         'condprob' => $condprob,
         'fpscr' => $fpscr,
         'numinsns' => $numinsns,
+        'randseed' => $randseed,
         'fp_enabled' => $fp_enabled,
         'sve_enabled' => $sve_enabled,
         'outfile' => $outfile,
diff --git a/risugen_arm.pm b/risugen_arm.pm
index 28ca72f..2dc144d 100644
--- a/risugen_arm.pm
+++ b/risugen_arm.pm
@@ -1069,7 +1069,7 @@ sub gen_one_insn($$)
     }
 }
 
-sub write_test_code($$$$$$$$)
+sub write_test_code($$$$$$$$$)
 {
     my ($params) = @_;
 
@@ -1090,6 +1090,7 @@ sub write_test_code($$$$$$$$)
     my $condprob = $params->{ 'condprob' };
     my $fpscr = $params->{ 'fpscr' };
     my $numinsns = $params->{ 'numinsns' };
+    my $randseed = $params->{ 'randseed' };
     my $fp_enabled = $params->{ 'fp_enabled' };
     my $sve_enabled = $params->{ 'sve_enabled' };
     my $outfile = $params->{ 'outfile' };
@@ -1103,8 +1104,12 @@ sub write_test_code($$$$$$$$)
     # probability of forcing insn to unconditional
     $condprob = 1 - $condprob;
 
-    # TODO better random number generator?
-    srand(0);
+    # use time() as random gen seed by default
+    if ($randseed == -1) {
+        srand(time());
+    } else {
+        srand($randseed);
+    }
 
     print "Generating code using patterns: @keys...\n";
     progress_start(78, $numinsns);
diff --git a/risugen_loongarch64.pm b/risugen_loongarch64.pm
index 5394fdc..3b1b4f9 100644
--- a/risugen_loongarch64.pm
+++ b/risugen_loongarch64.pm
@@ -461,6 +461,7 @@ sub write_test_code($)
     my $condprob = $params->{ 'condprob' };
     my $fcsr = $params->{'fpscr'};
     my $numinsns = $params->{ 'numinsns' };
+    my $randseed = $params->{ 'randseed' };
     my $fp_enabled = $params->{ 'fp_enabled' };
     my $outfile = $params->{ 'outfile' };
 
@@ -473,8 +474,12 @@ sub write_test_code($)
     # probability of forcing insn to unconditional
     $condprob = 1 - $condprob;
 
-    # TODO better random number generator?
-    srand(0);
+    # use time() as random gen seed by default
+    if ($randseed == -1) {
+        srand(time());
+    } else {
+        srand($randseed);
+    }
 
     print "Generating code using patterns: @keys...\n";
     progress_start(78, $numinsns);
diff --git a/risugen_m68k.pm b/risugen_m68k.pm
index 7d62b13..85fc3da 100644
--- a/risugen_m68k.pm
+++ b/risugen_m68k.pm
@@ -158,6 +158,7 @@ sub write_test_code($)
 
     my $condprob = $params->{ 'condprob' };
     my $numinsns = $params->{ 'numinsns' };
+    my $randseed = $params->{ 'randseed' };
     my $outfile = $params->{ 'outfile' };
 
     my %insn_details = %{ $params->{ 'details' } };
@@ -172,8 +173,12 @@ sub write_test_code($)
     # probability of forcing insn to unconditional
     $condprob = 1 - $condprob;
 
-    # TODO better random number generator?
-    srand(0);
+    # use time() as random gen seed by default
+    if ($randseed == -1) {
+        srand(time());
+    } else {
+        srand($randseed);
+    }
 
     print "Generating code using patterns: @keys...\n";
     progress_start(78, $numinsns);
diff --git a/risugen_ppc64.pm b/risugen_ppc64.pm
index b241172..4bc2d62 100644
--- a/risugen_ppc64.pm
+++ b/risugen_ppc64.pm
@@ -368,9 +368,9 @@ sub write_test_code($)
 
     my $condprob = $params->{ 'condprob' };
     my $numinsns = $params->{ 'numinsns' };
+    my $randseed = $params->{ 'randseed' };
     my $fp_enabled = $params->{ 'fp_enabled' };
     my $outfile = $params->{ 'outfile' };
-
     my %insn_details = %{ $params->{ 'details' } };
     my @keys = @{ $params->{ 'keys' } };
 
@@ -384,8 +384,12 @@ sub write_test_code($)
     # probability of forcing insn to unconditional
     $condprob = 1 - $condprob;
 
-    # TODO better random number generator?
-    srand(0);
+    # use time() as random gen seed by default
+    if ($randseed == -1) {
+        srand(time());
+    } else {
+        srand($randseed);
+    }
 
     print "Generating code using patterns: @keys...\n";
     progress_start(78, $numinsns);
-- 
2.34.1

Re: [PATCH risu] use time() as random seed and introduce --randseed option

[Alex Bennée]

Jun Sun <jsun@junsun.net> writes:

> By default, risu currently does not generate random instruction sequences because it uses 0 as the random seed. 
> This patch uses time() as random seed and also introduces --randomseed option for deterministic sequence
> generation.

I can see the benefit for being able to change the seed but I think
using time() by default means any given sequence won't be reproducible.
This is useful behaviour if you want to regenerate the same test
sequence on another machine without copying stuff about.

>
> [4. text/x-diff; 0008-add-randseed-option-and-use-time-as-default-seed.patch]...


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH risu] use time() as random seed and introduce --randseed option

[Jun Sun]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

Agree on the usefulness of generating the same test.  That is the reason
behind adding --randseed option.  Once a seed is set, it always generates
the same sequence of instructions.

Basically with this patch,

   - by default you will generate random instruction sequences for most
   testing cases
   - you can provide a random seed option in the commandline to generate a
   deterministic instruction sequence

Without this patch,

   - we always get one fixed sequence (ie. random seed == 0 case)
   - Otherwise we would have to manually modify code to generate random
   instruction sequences or generate a different fixed sequence.

Hope this clarifies things a little bit.

Jun

On Wed, May 3, 2023 at 10:05 AM Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Jun Sun <jsun@junsun.net> writes:
>
> > By default, risu currently does not generate random instruction
> sequences because it uses 0 as the random seed.
> > This patch uses time() as random seed and also introduces --randomseed
> option for deterministic sequence
> > generation.
>
> I can see the benefit for being able to change the seed but I think
> using time() by default means any given sequence won't be reproducible.
> This is useful behaviour if you want to regenerate the same test
> sequence on another machine without copying stuff about.
>
> >
> > [4. text/x-diff;
> 0008-add-randseed-option-and-use-time-as-default-seed.patch]...
>
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

[-- Attachment #2: Type: text/html, Size: 2007 bytes --]

Re: [PATCH risu] use time() as random seed and introduce --randseed option

[Peter Maydell]

On Fri, 5 May 2023 at 01:23, Jun Sun <jsun@junsun.net> wrote:
>
> Agree on the usefulness of generating the same test.  That is the reason behind adding --randseed option.  Once a seed is set, it always generates the same sequence of instructions.
>
> Basically with this patch,
>
> by default you will generate random instruction sequences for most testing cases
> you can provide a random seed option in the commandline to generate a deterministic instruction sequence
>
> Without this patch,
>
> we always get one fixed sequence (ie. random seed == 0 case)
> Otherwise we would have to manually modify code to generate random instruction sequences or generate a different fixed sequence.
>
> Hope this clarifies things a little bit.

Mmm; it comes down to: should we default to 'time' and
require the user to specify --randseed 0 to get the old
behaviour; or do we retain the current behaviour as the
default and let the user pass an option if they want a
non-reproducibly random output.

Alex, what do you reckon? You probably have been using
risugen more actively than me recently. I guess I vaguely
lean to "default to randomize(time)".

Also, should we make risugen print the random seed to stdout
so you can repro it even if you didn't pass --randseed initially?

Now that the random-seed-setting is 6 lines instead of 1,
this should definitely be abstracted out to a function
in the common code and not repeated in each per-arch file.

thanks
-- PMM

Re: [PATCH risu] use time() as random seed and introduce --randseed option

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 5 May 2023 at 01:23, Jun Sun <jsun@junsun.net> wrote:
>>
>> Agree on the usefulness of generating the same test. That is the
>> reason behind adding --randseed option. Once a seed is set, it
>> always generates the same sequence of instructions.
>>
>> Basically with this patch,
>>
>> by default you will generate random instruction sequences for most testing cases
>> you can provide a random seed option in the commandline to generate a deterministic instruction sequence
>>
>> Without this patch,
>>
>> we always get one fixed sequence (ie. random seed == 0 case)
>> Otherwise we would have to manually modify code to generate random
>> instruction sequences or generate a different fixed sequence.
>>
>> Hope this clarifies things a little bit.
>
> Mmm; it comes down to: should we default to 'time' and
> require the user to specify --randseed 0 to get the old
> behaviour; or do we retain the current behaviour as the
> default and let the user pass an option if they want a
> non-reproducibly random output.
>
> Alex, what do you reckon? You probably have been using
> risugen more actively than me recently. I guess I vaguely
> lean to "default to randomize(time)".

I'm easy either way as long as we as long as we print out the seed so we
can deterministically regenerate if we want to.

>
> Also, should we make risugen print the random seed to stdout
> so you can repro it even if you didn't pass --randseed initially?
>
> Now that the random-seed-setting is 6 lines instead of 1,
> this should definitely be abstracted out to a function
> in the common code and not repeated in each per-arch file.
>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH risu] use time() as random seed and introduce --randseed option

[Jun Sun]

[-- Attachment #1: Type: text/plain, Size: 2202 bytes --]

It seems to me that we are OK with random as default behavior but we should
print out the seed (and abstract it into a shared function across arches).

Happy to re-work the patch if my understanding is correct.

Jun

On Thu, May 18, 2023 at 6:09 AM Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 5 May 2023 at 01:23, Jun Sun <jsun@junsun.net> wrote:
> >>
> >> Agree on the usefulness of generating the same test. That is the
> >> reason behind adding --randseed option. Once a seed is set, it
> >> always generates the same sequence of instructions.
> >>
> >> Basically with this patch,
> >>
> >> by default you will generate random instruction sequences for most
> testing cases
> >> you can provide a random seed option in the commandline to generate a
> deterministic instruction sequence
> >>
> >> Without this patch,
> >>
> >> we always get one fixed sequence (ie. random seed == 0 case)
> >> Otherwise we would have to manually modify code to generate random
> >> instruction sequences or generate a different fixed sequence.
> >>
> >> Hope this clarifies things a little bit.
> >
> > Mmm; it comes down to: should we default to 'time' and
> > require the user to specify --randseed 0 to get the old
> > behaviour; or do we retain the current behaviour as the
> > default and let the user pass an option if they want a
> > non-reproducibly random output.
> >
> > Alex, what do you reckon? You probably have been using
> > risugen more actively than me recently. I guess I vaguely
> > lean to "default to randomize(time)".
>
> I'm easy either way as long as we as long as we print out the seed so we
> can deterministically regenerate if we want to.
>
> >
> > Also, should we make risugen print the random seed to stdout
> > so you can repro it even if you didn't pass --randseed initially?
> >
> > Now that the random-seed-setting is 6 lines instead of 1,
> > this should definitely be abstracted out to a function
> > in the common code and not repeated in each per-arch file.
> >
> > thanks
> > -- PMM
>
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

[-- Attachment #2: Type: text/html, Size: 2940 bytes --]

Re: [PATCH risu] use time() as random seed and introduce --randseed option

[Alex Bennée]

Jun Sun <jsun@junsun.net> writes:

> It seems to me that we are OK with random as default behavior but we should print out the seed (and abstract it into
> a shared function across arches).
>
> Happy to re-work the patch if my understanding is correct.

Works for me. Please respin.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro