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

[Ajit Agarwal <aagarwa1@linux.ibm.com>]

Hello Jeff:

On 16/03/23 8:18 pm, Jeff Law wrote:
> 
> 
> 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
> 

Thanks for your advice. At the input of REE pass the RTL has the following wherein zero_extend and subreg( reg 117) is converted to and (subreg DI ( reg QI 117).

This needs to be handled. I am working on handling this in REE pass. 

insn 44 43 18 3 (set (reg:SI 122)
        (if_then_else:SI (le:SI (reg:CC 130)
                (const_int 0 [0]))
            (reg:SI 129)
            (const_int 0 [0]))) "ext.cc":5:5 -1
     (nil))
(insn 18 44 40 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)))
(jump_insn 40 18 41 3 (set (pc)
        (label_ref 19)) -1
     (nil)
 -> 19)
(barrier 41 40 32)
(code_label 32 41 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) [1 uses])
(note 20 19 21 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
(note 21 20 26 5 NOTE_INSN_DELETED)
(insn 26 21 27 5 (set (reg/i:DI 3 %r3)
        (and:DI (subreg:DI (reg:QI 117 [ _1 ]) 0)
            (const_int 1 [0x1]))) "ext.cc":8:1 207 {anddi3_mask}
     (expr_list:REG_DEAD (reg:QI 117 [ _1 ])
        (nil)))
(insn 27 26 0 5 (use (reg/i:DI 3 %r3)) "ext.cc":8:1 -1
     (nil))
"a-ext.cc.292r.split1" 92L, 4727C                     

Thanks & Regards
Ajit