Re: [Qemu-devel] x86: cvtsi2s{s,d} etc. array access

[Blue Swirl <blauwirbel@gmail.com>]

On Tue, May 15, 2012 at 5:27 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 May 2012 22:05, Blue Swirl <blauwirbel@gmail.com> wrote:
>> While working on the AREG0 patches, I noticed strange code in
>> target-i386/translate.c.
>
>> It's accessed like this (line 3537):
>>            sse_op2 = sse_op_table3[(s->dflag == 2) * 2 + ((b >> 8) - 2)];
>>
>> b >> 8 can be only either 1 or 0.
>
> I don't think this is true. At this point in the code we're inside
> a "switch (b)" so we know that b is either 0x22a (cvtsi2ss) or
> 0x32a (cvtsi2sd). So "((b >> 8) - 2)" is 0 for cvtsi2ss and 1
> for cvtsi2sd, giving us the lsbit of the array index, with
> (s->dflag == 2) providing the next bit, so we end up with
> indexes 0,1,2,3 in this table for these two insns in their
> doubleword and quadword forms.

OK, I misread the start of the function pretty badly.

>
> You could rewrite "((b >> 8) - 2)" as "((b >> 8) & 1)".
>
>> The other access is as follows (line 3594):
>>            sse_op2 = sse_op_table3[(s->dflag == 2) * 2 + ((b >> 8) - 2) + 4 +
>>                                    (b & 1) * 4];
>>
>> This looks better because of + 4 but I think some array values are not
>> accessible (max. 1 * 2 + (1 - 2) + 4 + 1 * 4 == 9).
>
> Here we know b is 0x22c (cvttss2si) 0x32c (cvttsd2si) 0x22d (cvtss2si)
> or 0x32d (cvtsd2si). ((b >> 8) - 2) distinguishes the 0x2XX and 0x3XX,
> and (b & 1) the 0xXXc from 0xXXd. So the index is made up of (lsbit to
> msbit) "0x2XX or 0x3XX?", "double or quad?", "0xXXC or 0xXXD?", and then
> we add a constant offset of 4 because the entries start after the
> 4 entries for the cases we looked at earlier.
>
> I think you could actually split sse_op_table3 into two separate
> tables, one for each of these cases, which would be slightly
> clearer IMHO.

Yes, this is IMHO ugly and there is no type safety due to void
pointers. There could be also an inner switch, like how cvttps2pi is
handled nearby.

>
> -- PMM