Re: [Qemu-devel] [RISU RFC PATCH v1 1/7] risugen_common: add insnv, randint_constr, rand_fill

[Jan Bobek <jan.bobek@gmail.com>]

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

On 6/27/19 4:53 AM, Richard Henderson wrote:
> On 6/19/19 7:04 AM, Jan Bobek wrote:
>> +        my $value = ($args{bigendian}
>> +                     ? ($args{value} >> (8 * $args{len} - $bitlen))
>> +                     : $args{value});
> ...
>> +        $args{value} >>= $bitlen unless $args{bigendian};
> 
> I think this could be clearer without modifying $args.
> I mis-read these the first time around.
> 
> Perhaps
> 
>     my $bitpos = 0;
>     my $bitend = 8 * $args{len};
>     while ($bitpos < $bitend) {
>         ...
>         my $value = $args{value} >> ($args{bigendian}
>                                      ? $bitend - $bitpos - $bitlen
>                                      : $bitpos);
>         ...
>         $bitpos += $bitlen;
>     }

Looks good, I'll change it.

>> +sub randint_constr(%)
>> +{
>> +    my (%args) = @_;
>> +    my $bitlen = $args{bitlen};
>> +    my $halfrange = 1 << ($bitlen - 1);
>> +
>> +    while (1) {
>> +        my $value = int(rand(2 * $halfrange));
>> +        $value -= $halfrange if defined $args{signed} && $args{signed};
>> +        $value &= ~$args{fixedbitmask} if defined $args{fixedbitmask};
>> +        $value |= $args{fixedbits} if defined $args{fixedbits};
>> +
>> +        if (defined $args{constraint}) {
>> +            if (!($args{constraint} >> 63)) {
>> +                $value = $args{constraint};
>> +            } elsif ($value == ~$args{constraint}) {
>> +                next;
>> +            }
>> +        }
> 
> I don't understand what you're doing here with {constraint}.
> Some additional commentary would help.

The idea is: if the most significant bit of $args{constraint} is zero,
$args{constraint} is the value we want to return; if the most
significant bit is one, ~$args{constraint} (its bit inversion) is the
value we want to *avoid*, so we try again. This is used to to
implement constraints on fields such as

MOVLPS          SSE     00001111 0001001 d !emit { modrm(mod => ~MOD_DIRECT); mem(size => 8); }
MOVLHPS         SSE     00001111 00010110  !emit { modrm(mod => MOD_DIRECT); }

The bitshift by 63 assumes 64-bit integers, but that assumption is
present in other places, too. I couldn't think of a better way to do
it: comparing it to zero doesn't work because the value is unsigned.
I will include a comment explaining this in v2, unless you have
other suggestions.

-Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]