Re: [PATCH] rs6000: suboptimal code for returning bool value on target ppc

[Jeff Law <jeffreyalaw@gmail.com>]

On 3/16/23 04:11, Ajit Agarwal via Gcc-patches wrote:
> 
> Hello Richard:
> 
> On 16/03/23 3:22 pm, Richard Biener wrote:
>> On Thu, Mar 16, 2023 at 9:19 AM Ajit Agarwal <aagarwa1@linux.ibm.com> wrote:
>>>
>>>
>>>
>>> On 16/03/23 1:44 pm, Richard Biener wrote:
>>>> On Thu, Mar 16, 2023 at 9:11 AM Ajit Agarwal <aagarwa1@linux.ibm.com> wrote:
>>>>>
>>>>> Hello Richard:
>>>>>
>>>>> On 16/03/23 1:10 pm, Richard Biener wrote:
>>>>>> On Thu, Mar 16, 2023 at 6:21 AM Ajit Agarwal via Gcc-patches
>>>>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>>>>
>>>>>>> Hello All:
>>>>>>>
>>>>>>>
>>>>>>> This patch eliminates unnecessary zero extension instruction from power generated assembly.
>>>>>>> Bootstrapped and regtested on powerpc64-linux-gnu.
>>>>>>
>>>>>> What makes this so special that we cannot deal with it from generic code?
>>>>>> In particular we do have the REE pass, why is target specific
>>>>>> knowledge neccessary
>>>>>> to eliminate the extension?
>>>>>>
>>>>>
>>>>> For returning bool values and comparision with integers generates the following by all the rtl passes.
>>>>>
>>>>> set compare (subreg)
>>>>> set if_then_else
>>>>> Convert SImode -> QImode
>>>>> set zero_extend to SImode from QImode
>>>>> set return value 0 in one path of cfg.
>>>>> set return value 1 in other path of cfg.
>>>>>
>>>>> This pass replaces the above zero extension and conversion from QImode to DImode with copy operation to keep QImode in 64 bit registers in powerpc target.
>>>>
>>>> Sorry, I can't parse that - as there's no testcase with the patch I
>>>> cannot even try to see what the actual RTL
>>>> looks like (without the pass).
>>>>
>>>
>>> Here is the PR with bugzilla.
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103784
>>>
>>> I can add the attached testcase with this PR in the patch.
>>
>> I don't see any zero-extends there.
>>
> 
> Here is the testcase.
> 
> 
> bool (int a, int b)
> {
>            if (a > 2)
>                        return false;
>             if (b < 10)
>                         return true;
>               return false;
> }
> 
> compiled with gcc -O3 -m64 testcase.cc -mcpu=power9 -save-temps.
> 
> Here is the rtl after cse.
> (note 12 11 15 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
> (insn 15 12 16 3 (set (reg:CC 123)
>          (compare:CC (subreg/s/u:SI (reg/v:DI 120 [ b ]) 0)
>              (const_int 9 [0x9]))) "ext.cc":5:5 796 {*cmpsi_signed}
>       (expr_list:REG_DEAD (reg/v:DI 120 [ b ])
>          (nil)))
> (insn 16 15 17 3 (set (reg:SI 124)
>          (const_int 1 [0x1])) "ext.cc":5:5 555 {*movsi_internal1}
>       (nil))
> (insn 17 16 18 3 (set (reg:SI 122)
>          (if_then_else:SI (gt (reg:CC 123)
>                  (const_int 0 [0]))
>              (const_int 0 [0])
>              (reg:SI 124))) "ext.cc":5:5 344 {isel_cc_si}
>       (expr_list:REG_DEAD (reg:SI 124)
>          (expr_list:REG_DEAD (reg:CC 123)
>              (nil))))
> (insn 18 17 32 3 (set (reg:QI 117 [ _1 ])
>          (subreg:QI (reg:SI 122) 0)) "ext.cc":5:5 562 {*movqi_internal}
>       (expr_list:REG_DEAD (reg:SI 122)
>          (nil)))
>        ; pc falls through to BB 5
> (code_label 32 18 31 4 3 (nil) [1 uses])
> (note 31 32 5 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
> (insn 5 31 19 4 (set (reg:QI 117 [ _1 ])
>          (const_int 0 [0])) "ext.cc":4:16 562 {*movqi_internal}
>       (nil))
> (code_label 19 5 20 5 2 (nil) [0 uses])
> (note 20 19 21 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
> (insn 21 20 22 5 (set (reg:DI 126 [ _1 ])
>          (zero_extend:DI (reg:QI 117 [ _1 ]))) "ext.cc":8:1 5 {zero_extendqidi2}
>       (expr_list:REG_DEAD (reg:QI 117 [ _1 ])
>          (nil)))
> (insn 22 21 26 5 (set (reg:DI 118 [ <retval> ])
>          (reg:DI 126 [ _1 ])) "ext.cc":8:1 681 {*movdi_internal64}
>       (expr_list:REG_DEAD (reg:DI 126 [ _1 ])
>          (nil)))
> (insn 26 22 27 5 (set (reg/i:DI 3 3)
>          (reg:DI 126 [ _1 ])) "ext.cc":8:1 681 {*movdi_internal64}
>       (expr_list:REG_DEAD (reg:DI 118 [ <retval> ])
>          (nil)))
> (insn 27 26 0 5 (use (reg/i:DI 3 3)) "ext.cc":8:1 -1
>       (nil))
This looks like it'd be better addressed in REE.


We've got two paths to the zero_extend.  One sets (reg 117) from a 
constant.  The other sets (reg 117) from a (subreg:QI (reg:SI)).

Handling the constant is trivial.  For the other set, we can replace the 
subreg with the zero_extend.  Presumably we'd then proceed to try and 
eliminate the zero-extend by realizing both arms of the conditional move 
are constants and thus trivially handled.

While I don't think REE would handle all this today, fixing it to handle 
this case seems like it'd be better than doing a specialized pass in the 
ppc backend.

jeff