Re: target/ppc: bug in optimised vsl/vsr implementation?

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

28.09.2019. 19.45, "Aleksandar Markovic" <aleksandar.m.mail@gmail.com> је
написао/ла:
>
>
> 26.09.2019. 20.14, "Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk> је
написао/ла:
> >
> > As part of the investigation into the DFP number issue reported at
> > https://bugs.launchpad.net/qemu/+bug/1841990 it appears that there may
also be a bug
> > introduced by the new optimised vsl/vsr implementation:
> >
> > commit 4e6d0920e7547e6af4bbac5ffe9adfe6ea621822
> > Author: Stefan Brankovic <stefan.brankovic@rt-rk.com>
> > Date: Mon Jul 15 16:22:48 2019 +0200
> >
> >     target/ppc: Optimize emulation of vsl and vsr instructions
> >
> >
>
> (sorry in advance if the format of this message looks odd, I have some
problems with mail settings related to recent qemu-devel mailer settings
changes; I will adjust my mail settings in next few days)
>
> Mark and Paul (and Stefan),
>
> Thanks for spotting this and pinpointing the culprit commit. I guess
Stefan is going to respond soon, but, in the meantime, I took a look at the
commit in question:
>
>
https://github.com/qemu/qemu/commit/4e6d0920e7547e6af4bbac5ffe9adfe6ea621822
>
> I don't have at the moment any dev/test environment handy, but I did
manual inspection of the code, and here is what I found (in order of
importance, perceived by me):
>
> 1. The code will not work correctly if the shift ammount (variable 'sh')
is 0. This is because, in that case, one of succeeding invocations of TCG
shift functions will be required to shift a 64-bit TCG variable by 64 bits,
and the result of such TCG operation is undefined (shift amount must be 63
or less) - see https://github.com/qemu/qemu/blob/master/tcg/README.
>
> 2. Variable naming is better in the old helper than in the new
translator. In that light, I would advise Stefan to change 'sh' to 'shift',
and 'shifted' to 'carry'.
>
> 3. Lines
>
> tcg_gen_andi_i64(shifted, shifted, 0x7fULL);
>
> and
>
> tcg_gen_andi_i64(shifted, shifted, 0xfe00000000000000ULL);
>
> appear to be spurious (albait in a harmless way). Therefore, they should
be deleted, or, alternatevely, a justification for them should be provided.
>
> 4. In the commit message, variable names were used without quotation
mark, resulting in puzzling and unclear wording.
>
> 5. (a question for Mark) After all recent changes, does get_avr64(...,
..., true) always (for any endian configuration) return the "high" half of
an Altivec register, and get_avr64(..., ..., false) the "low" one?
>

One more hint: variables 'avrA' and 'avrB' can be a single variable, since
there is no moment during execution where both are needed; the same for
'tmp' and 'shifted'.

Also, check on the hardware the behavior listed as 'undefined' for vsl/vsr
in the docs - even though it is tehnically irrelevant, I am courious
whether the old or the new (or none of them) solution match the hardware.

> Given all these circumstances, perhaps the most reasonable solution would
be to revert the commit in question, and allow Stefan enough dev and test
time to hopefully submit a new, better, version later on.
>
> Sincerely,
> Aleksandar
>

[-- Attachment #2: Type: text/html, Size: 4128 bytes --]