Re: [PATCH] target/ppc: Fix for optimized vsl/vsr instructions

[Mark Cave-Ayland]

On 04/10/2019 19:41, Paul Clarke wrote:

> On 10/4/19 2:33 AM, Mark Cave-Ayland wrote:
>> On 03/10/2019 11:10, Stefan Brankovic wrote:
>>> In previous implementation, invocation of TCG shift function could request
>>> shift of TCG variable by 64 bits when variable "sh" is 0, which is not
>>> supported in TCG (values can be shifted by 0 to 63 bits). This patch fixes
>>> this by using two separate invocation of TCG shift functions, with maximum
>>> shift amount of 32.
>>>     
>>> Also, name of variable "shifted" is changed to "carry" so variable naming
>>> is similar to old helper implementation.
>>>
>>> Stefan Brankovic (1):
>>>   target/ppc: Fix for optimized vsl/vsr instructions
>>>
>>>  target/ppc/translate/vmx-impl.inc.c | 40 
>>> ++++++++++++++++++-------------------
>>>  1 file changed, 20 insertions(+), 20 deletions(-)
>>
>> Thanks Stefan! Superficially this looks okay to me (although it's probably 
>> worth
>> adding a "Fixes" tag to the commit message) however I should add that I was 
>> only
>> aware of it because Paul discovered this as part of his ppc64le tests - my
>> involvement was really to fix up the DFP bits that were broken by one of my 
>> earlier
>> patches.
>>
>> From my perspective if David and Richard are happy then that's fine with me. 
>> Paul, in
>> terms of your tests do you still see the bcdadd and bcdsub failures running 
>> them on
>> David's ppc-for-4.2 branch plus this patch?
> 
> Applying this patch on top of dce5a787c05fe1a3e54d92871cdeba2af6798e0d 
> eliminated the failures that I reported in 
> https://bugs.launchpad.net/qemu/+bug/1841990 associated with vsl/vsr (which I 
> presume is what you were referring to instead of bcdadd/bcdsub).

Yes indeed - I was just commenting that based upon
https://bugs.launchpad.net/qemu/+bug/1841990/comments/13 then even with the DFP 
and
vsl/vsr fixes applied (now present in David's ppc-for-4.2 branch) there is 
still an
error somewhere in QEMU's bcdadd and bcdsub implementation.

I'm fairly sure that this is an existing bug though.


ATB,

Mark.