Re: [PATCH 2/3]rs6000: NFC use sext_hwi to replace ((v&0xf..f)^0x80..0) - 0x80..0

[Jiufu Guo <guojiufu@linux.ibm.com>]

Hi Kewen,

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> on 2022/12/1 20:16, guojiufu wrote:
>> On 2022-12-01 15:10, Jiufu Guo via Gcc-patches wrote:
>>> Hi Kewen,
>>>
>>> 在 12/1/22 2:11 PM, Kewen.Lin 写道:
>>>> on 2022/12/1 13:35, Jiufu Guo wrote:
>>>>> Hi Kewen,
>>>>>
>>>>> Thanks for your quick and insight review!
>>>>>
>>>>> 在 12/1/22 1:17 PM, Kewen.Lin 写道:
>>>>>> Hi Jeff,
>>>>>>
>>>>>> on 2022/12/1 09:36, Jiufu Guo wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> This patch just uses sext_hwi to replace the expression like:
>>>>>>> ((value & 0xf..f) ^ 0x80..0) - 0x80..0 for rs6000.cc and rs6000.md.
>>>>>>>
>>>>>>> Bootstrap & regtest pass on ppc64{,le}.
>>>>>>> Is this ok for trunk?
>>>>>>
>>>>>> You didn't say it clearly but I guessed you have grepped in the whole
>>>>>> config/rs6000 directory, right?  I noticed there are still two places
>>>>>> using this kind of expression in function constant_generates_xxspltiw,
>>>>>> but I assumed it's intentional as their types are not HOST_WIDE_INT.
>>>>>>
>>>>>> gcc/config/rs6000/rs6000.cc:      short sign_h_word = ((h_word & 0xffff) ^ 0x8000) - 0x8000;
>>>>>> gcc/config/rs6000/rs6000.cc:  int sign_word = ((word & 0xffffffff) ^ 0x80000000) - 0x80000000;
>>>>>>
>>>>>> If so, could you state it clearly in commit log like "with type
>>>>>> signed/unsigned HOST_WIDE_INT" or similar?
>>>>>>
>>>>> Good question!
>>>>>
>>>>> And as you said sext_hwi is more for "signed/unsigned HOST_WIDE_INT".
>>>>> For these two places, it seems sext_hwi is not needed actually!
>>>>> And I did see why these expressions are used, may be just an assignment
>>>>> is ok.
>>>>
>>>> ah, I see.  I agree using the assignment is quite enough.  Could you
>>>> please also simplify them together?  Since they are with the form
>>>> "((value & 0xf..f) ^ 0x80..0) - 0x80..0" too, and can be refactored
>>>> in a better way.  Thanks!
>>>
>>> Sure, I believe just "short sign_h_word = vsx_const->half_words[0];"
>>> should be correct :-), and included in the updated patch.
>>>
>>> Updated patch is attached,  bootstrap&regtest is on going.
>> 
>
> on 2022/12/1 15:10, Jiufu Guo wrote:
>> From 8aa8e1234b6ec34473434951a3a6177253aac770 Mon Sep 17 00:00:00 2001
>> From: Jiufu Guo <guojiufu@linux.ibm.com>
>> Date: Wed, 30 Nov 2022 13:13:37 +0800
>> Subject: [PATCH 2/2]rs6000: update ((v&0xf..f)^0x80..0) - 0x80..0 with code: like sext_hwi
>> 
>
> May be shorter with "rs6000: Update sign extension computation with
> sext_hwi"?
Thanks for your great suggestions!
>
>> This patch just replaces the expression like: 
>> ((value & 0xf..f) ^ 0x80..0) - 0x80..0 to better code(e.g. sext_hwi) for
>> rs6000.cc, rs6000.md and predicates.md (files under rs6000/).
>
>
>> Bootstrap and regtest pass on ppc64{,le}.
>> 
>
> Thanks for updating and testing, this patch is OK.

BR,
Jeff (Jiufu)

>
> BR,
> Kewen