Re: [risu PATCH 1/2] risugen: allow instructions with length > 32 bit

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 27 Oct 2023 at 11:05, Thomas Huth <thuth@redhat.com> wrote:
>
> RISU currently only supports instructions with a length of
> 16 bit or 32 bit, however classical CISC systems like s390x
> also have instructions that are longer than 32 bit. Thus let's
> change the generator to support longer instructions, too.
>
> This adds support for 48-bit instructions on s390x, while
> the other architectures are just minimally changed to preserve
> the current state.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  risugen                | 48 +++++++++++++++++++++++++++++-------------
>  risugen_arm.pm         |  6 +++---
>  risugen_common.pm      |  2 +-
>  risugen_loongarch64.pm |  4 ++--
>  risugen_m68k.pm        |  6 +++---
>  risugen_ppc64.pm       |  4 ++--
>  risugen_s390x.pm       | 17 +++++++++++----
>  7 files changed, 57 insertions(+), 30 deletions(-)
>
> diff --git a/risugen b/risugen
> index fa94a39..833b459 100755
> --- a/risugen
> +++ b/risugen
> @@ -105,6 +105,16 @@ sub read_tokenised_line(*)
>      return @tokens;
>  }
>
> +sub check_bitmask($$)
> +{
> +    my ($fixedbits, $fixedbitmask) = @_;
> +
> +    if ((($fixedbits & $fixedbitmask) != $fixedbits)
> +        || (($fixedbits & ~$fixedbitmask) != 0)) {
> +        die "internal error: fixed bits not lined up with mask";
> +    }
> +}
> +
>  sub parse_config_file($)
>  {
>      # Read in the config file defining the instructions we can generate
> @@ -160,10 +170,11 @@ sub parse_config_file($)
>              exit(1);
>          }
>
> -        my $fixedbits = 0;
> -        my $fixedbitmask = 0;
> +        my @fixedbits = (0, 0, 0, 0);
> +        my @fixedbitmask = (0, 0, 0, 0);

I wonder if rather than turning these into arrays of integers, we
should use Perl bit vectors (see 'perldoc -f vec'). You can use those
to create arbitrary length bit-strings, and they directly support
the usual bitwise logical operators.

It's probably a bit of a bigger conversion job, though (and I
haven't needed to use them before, so there might be awkwardnesses
I haven't anticipated).

>          my $bitpos = 32;
> -        my $insnwidth = 32;
> +        my $wordpos = 0;
> +        my $insnwidth = 0;
>          my $seenblock = 0;
>
>          while (@bits) {
> @@ -217,36 +228,43 @@ sub parse_config_file($)
>
>              my $bitmask = oct("0b". '1' x $bitlen);
>              $bitpos -= $bitlen;
> +            $insnwidth += $bitlen;
>              if ($bitpos < 0) {
>                  print STDERR "$file:$.: ($insn $enc) too many bits specified\n";
>                  exit(1);
>              }
>
>              if (defined $bitval) {
> -                $fixedbits |= ($bitval << $bitpos);
> -                $fixedbitmask |= ($bitmask << $bitpos);
> +                $fixedbits[$wordpos] |= ($bitval << $bitpos);
> +                $fixedbitmask[$wordpos] |= ($bitmask << $bitpos);
>              } else {
> -                push @fields, [ $var, $bitpos, $bitmask ];
> +                push @fields, [ $var, $bitpos, $bitmask, $wordpos ];
> +            }
> +
> +            if ($bitpos == 0) {
> +                check_bitmask($fixedbits[$wordpos], $fixedbitmask[$wordpos]);
> +
> +                $wordpos += 1;
> +                if (@bits) {
> +                    $bitpos = 32;
> +                }
>              }
>          }
>          if ($bitpos == 16) {
>              # assume this is a half-width thumb instruction
>              # Note that we don't fiddle with the bitmasks or positions,
>              # which means the generated insn will be in the high halfword!

I suspect the process of conversion to bit-vectors will probably
imply fixing this bit of ugliness en route, incidentally.

> -            $insnwidth = 16;
> -        } elsif ($bitpos != 0) {
> -            print STDERR "$file:$.: ($insn $enc) not enough bits specified\n";
> +            check_bitmask($fixedbits[$wordpos], $fixedbitmask[$wordpos]);
> +        } elsif ($bitpos != 0 && $bitpos != 32) {
> +            print STDERR "$file:$.: ($insn $enc) not enough bits specified ($bitpos)\n";
>              exit(1);
>          }

thanks
-- PMM