Re: [Qemu-devel] [PATCH Risu 2/3] Isolates Arm specific subroutines out from risugen main file

[Peter Maydell]

On 28 October 2016 at 18:46, Jose Ricardo Ziviani
<address@hidden> wrote:
>  - In order to make risugen more agnostic to different archs, a new
>    module is created to handle Arm instrunctions. This module, as
>    well as the Power module, is dinamically called based on the
>    .mode directive defined in each risu file.
>
> Signed-off-by: Jose Ricardo Ziviani <address@hidden>
> ---
>  risugen        | 1018 ++---------------------------------------------------
>  risugen_arm.pm | 1075 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 1105 insertions(+), 988 deletions(-)
>  create mode 100644 risugen_arm.pm
>
> diff --git a/risugen b/risugen
> index 892a6dc..3c88c9b 100755
> --- a/risugen
> +++ b/risugen
> @@ -9,6 +9,7 @@
>  # Contributors:
>  #     Peter Maydell (Linaro) - initial implementation
>  #     Claudio Fontana (Linaro) - initial aarch64 support
> +#     Jose Ricardo Ziviani (IBM) - initial ppc64 support and arch isolation
>  
> ###############################################################################
>
>  # risugen -- generate a test binary file for use with risu
> @@ -17,12 +18,9 @@
>  use strict;
>  use Getopt::Long;
>  use Data::Dumper;
> +use Module::Load;
>  use Text::Balanced qw { extract_bracketed extract_multiple };
>
> -my $periodic_reg_random = 1;
> -my $enable_aarch64_ld1 = 0;
> -
> -my @insns;
>  my %insn_details;
>
>  # Note that we always start in ARM mode even if the C code was compiled for
> @@ -31,941 +29,21 @@ my %insn_details;
>  # an arm or thumb insn?); test_thumb tells us which mode we need to switch
>  # to to emit the insns under test.
>  # Use .mode aarch64 to start in Aarch64 mode.
> -
>  my $is_aarch64 = 0; # are we in aarch64 mode?
>  # For aarch64 it only makes sense to put the mode directive at the
>  # beginning, and there is no switching away from aarch64 to arm/thumb.
>
> -my $is_thumb = 0;   # are we currently in Thumb mode?
> +# Use .mode ppc64 to start in PowerPC64 mode.
> +my $is_ppc64 = 0; # are we in ppc64 mode?
> +

A patch that's just abstracting out the ARM stuff shouldn't
need to add an is_ppc64 flag.

> -sub write_test_code($$$$)
> -{
> -    my ($condprob, $fpscr, $numinsns, $fp_enabled) = @_;
> -    # convert from probability that insn will be conditional to
> -    # probability of forcing insn to unconditional
> -    $condprob = 1 - $condprob;
> -
> -    # TODO better random number generator?
> -    srand(0);
> -
> -    # Get a list of the insn keys which are permitted by the re patterns
> -    my @keys = keys %insn_details;
> -    if (@pattern_re) {
> -        my $re = '\b((' . join(')|(',@pattern_re) . '))\b';
> -        @keys = grep /$re/, @keys;
> -    }
> -    # exclude any specifics
> -    if (@not_pattern_re) {
> -        my $re = '\b((' . join(')|(',@not_pattern_re) . '))\b';
> -        @keys = grep !/$re/, @keys;
> -    }
> -    if (address@hidden) {
> -        print STDERR "No instruction patterns available! (bad config file or 
> --pattern argument?)\n";
> -        exit(1);
> -    }
> -    print "Generating code using patterns: @keys...\n";
> -    progress_start(78, $numinsns);
> -
> -    if ($fp_enabled) {
> -        write_set_fpscr($fpscr);
> -    }
> -
> -    if (grep { defined($insn_details{$_}->{blocks}->{"memory"}) } @keys) {
> -        write_memblock_setup();
> -    }
> -    # memblock setup doesn't clean its registers, so this must come 
> afterwards.
> -    write_random_register_data($fp_enabled);
> -    write_switch_to_test_mode();
> -
> -    for my $i (1..$numinsns) {
> -        my $insn_enc = $keys[int rand (@keys)];
> -        #dump_insn_details($insn_enc, $insn_details{$insn_enc});
> -        my $forcecond = (rand() < $condprob) ? 1 : 0;
> -        gen_one_insn($forcecond, $insn_details{$insn_enc});
> -        write_risuop($OP_COMPARE);
> -        # Rewrite the registers periodically. This avoids the tendency
> -        # for the VFP registers to decay to NaNs and zeroes.
> -        if ($periodic_reg_random && ($i % 100) == 0) {
> -            write_random_register_data($fp_enabled);
> -            write_switch_to_test_mode();
> -        }
> -        progress_update($i);
> -    }
> -    write_risuop($OP_TESTEND);
> -    progress_end();
> -}

This is all very close to being generic so it's odd to see it
get shifted out to the ARM-specific file entirely.

> @@ -1318,6 +341,7 @@ sub main()
>      # allow "--pattern re,re" and "--pattern re --pattern re"
>      @pattern_re = split(/,/,join(',',@pattern_re));
>      @not_pattern_re = split(/,/,join(',',@not_pattern_re));
> +    print "@not_pattern_re\n";

Stray debug print.

>
>      if ($#ARGV != 1) {
>          usage();
> @@ -1328,10 +352,28 @@ sub main()
>      $outfile = $ARGV[1];
>
>      parse_config_file($infile);
> -
> -    open_bin($outfile);
> -    write_test_code($condprob, $fpscr, $numinsns, $fp_enabled);
> -    close_bin();
> +
> +    my $module = 'risugen_arm';
> +    if ($is_ppc64 == 1) {
> +        $module = 'risugen_ppc64le';
> +    }
> +    load $module, qw/write_test_code/;

It would be nicer to accept an arbitrary string parameter
to a ".arch" keyword or something, and then look to see
whether a "risugen_$ARCH" module existed. Then every new
architecture can just drop its module file in place without
having to extend this code.

thanks
-- PMM