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

[Stefan Brankovic <stefan.brankovic@rt-rk.com>]

Please take a look at the following patch 
https://lists.nongnu.org/archive/html/qemu-ppc/2019-10/msg00133.html and 
let me know if problem is solved.

On 2.10.19. 16:08, Stefan Brankovic wrote:
> Hi Mark,
>
> Thank you for reporting this bug. I was away from office for couple of 
> days, so that's why I am answering you a bit late, sorry about that. I 
> will start working on a solution and try to fix this problem in next 
> couple of days.
>
> On 1.10.19. 20:24, Mark Cave-Ayland wrote:
>> On 28/09/2019 18:45, Aleksandar Markovic wrote:
>>
>> Hi Aleksandar,
>>
>> Thanks for taking a look at this!
>>
>>> 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.
>> Yes I think you're right here - the old helper got around this by 
>> doing an explicit
>> copy from a to r if the shift value is zero. In fact the case that 
>> Paul reported is
>> exactly this:
>>
>>     vsl VRT, VRA, VRB
>>
>> => 0x100006e0 <vec_slq+132>: vsl v0,v0,v1
>> (gdb) p $vr0.uint128
>> $21 = 0x10111213141516172021222324252650
>> (gdb) p $vr1.uint128
>> $22 = 0x0
>> (gdb) stepi
>> 0x00000000100006e4 in vec_slq ()
>> 1: x/i $pc
>> => 0x100006e4 <vec_slq+136>: xxlor vs0,vs32,vs32
>> (gdb) p $vr0.uint128
>> $23 = 0x10111213141516172021222324252650
>>
>> I guess the solution is check for sh == 0 and if this is the case, 
>> execute a copy
>> instead.
> I agree with you. This will be changed in upcoming patch.
>>
>>> 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'.
>> It looks like the name "sh" comes from the ISA documentation, so 
>> whilst it's a little
>> tricky to compare with the previous implementation it does make sense 
>> when comparing
>> with the algorithm shown there. Note: this implementation also drops 
>> the check for
>> each byte of VRB having the same shift value - should we care about 
>> this?
>
> "sh" is taken from the ISA documentation, so I would leave that as it 
> is now, but I can change some other variable names to be consistent 
> with previous implementation (e.g. "shifted" -> "carry").
>
> I don't think that we should check each byte of VRB, because we care 
> only about "defined" behavior. If shift values doesn't match, result 
> is "undefined" so it doesn't matter what is inside resulting register.
>
>>> 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.
>> I'm not sure why they are needed either - there's certainly no 
>> mention of it in the
>> ISA documentation. Stefan?
> This will be removed in upcoming patch.
>>
>>> 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?
>> Yes - the new functions always return the MSB (high) and LSB (low) 
>> correctly
>> regardless of host endian.
>>
>>> 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.
>> Given that it has been broken for 3 months now, I don't think we're 
>> in any major rush
>> to revert ASAP. I'd prefer to give Stefan a bit more time first since 
>> he does report
>> some substantial speed improvements from these new implementations.
>>
>>
>> ATB,
>>
>> Mark.
>
> Best Regards,
>
> Stefan
>