On 28 October 2016 at 18:46, Jose Ricardo Ziviani
<jos...@linux.vnet.ibm.com> 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 <jos...@linux.vnet.ibm.com>
> ---
> 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 (!@keys) {
> - 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
