Hello Jeff:
On 21/04/23 2:33 am, Ajit Agarwal via Gcc-patches wrote:
> Hello Jeff:
>
> On 20/04/23 3:23 am, Jeff Law wrote:
>>
>>
>> On 4/19/23 12:00, Ajit Agarwal wrote:
>>> Hello All:
>>>
>>> This is patch-3 to improve ree pass for rs6000 target.
>>> Main functionality routines to imprve ree pass.
>>>
>>> Bootstrapped and regtested on powerpc64-gnu-linux.
>>>
>>> Thanks & Regards
>>> Ajit
>>>
>>> ree: Improve ree pass for rs6000 target.
>>>
>>> For rs6000 target we see redundant zero and sign
>>> extension and done to improve ree pass to eliminate
>>> such redundant zero and sign extension. Support of
>>> zero_extend/sign_extend/AND.
>>>
>>> 2023-04-19 Ajit Kumar Agarwal <aagar...@linux.ibm.com>
>>>
>>> gcc/ChangeLog:
>>>
>>> * ree.cc (eliminate_across_bbs_p): Add checks to enable extension
>>> elimination across and within basic blocks.
>>> (def_arith_p): New function to check definition has arithmetic
>>> operation.
>>> (combine_set_extension): Modification to incorporate AND
>>> and current zero_extend and sign_extend instruction.
>>> (merge_def_and_ext): Add calls to eliminate_across_bbs_p and
>>> zero_extend sign_extend and AND instruction.
>>> (rtx_is_zext_p): New function.
>>> (reg_used_set_between_p): New function.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * g++.target/powerpc/zext-elim.C: New testcase.
>>> * g++.target/powerpc/zext-elim-1.C: New testcase.
>>> * g++.target/powerpc/zext-elim-2.C: New testcase.
>>> * g++.target/powerpc/sext-elim.C: New testcase.
>>> ---
>>> gcc/ree.cc | 451 ++++++++++++++++--
>>> gcc/testsuite/g++.target/powerpc/sext-elim.C | 18 +
>>> .../g++.target/powerpc/zext-elim-1.C | 19 +
>>> .../g++.target/powerpc/zext-elim-2.C | 11 +
>>> gcc/testsuite/g++.target/powerpc/zext-elim.C | 30 ++
>>> 5 files changed, 482 insertions(+), 47 deletions(-)
>>> create mode 100644 gcc/testsuite/g++.target/powerpc/sext-elim.C
>>> create mode 100644 gcc/testsuite/g++.target/powerpc/zext-elim-1.C
>>> create mode 100644 gcc/testsuite/g++.target/powerpc/zext-elim-2.C
>>> create mode 100644 gcc/testsuite/g++.target/powerpc/zext-elim.C
>>>
>>> diff --git a/gcc/ree.cc b/gcc/ree.cc
>>> index 413aec7c8eb..053db2e8ff3 100644
>>> --- a/gcc/ree.cc
>>> +++ b/gcc/ree.cc
>>> @@ -253,6 +253,71 @@ struct ext_cand
>>> static int max_insn_uid;
>>> +bool
>>> +reg_used_set_between_p (rtx set, rtx_insn *def_insn, rtx_insn *insn
>>> +{
>>> + if (reg_used_between_p (set, def_insn, insn)
>>> + || reg_set_between_p (set, def_insn, insn))
>>> + return true;
>>> +
>>> + return false;
>>> +}
>> This seems general enough that it should go into the same file as
>> reg_used_between_p and reg_set_between_p. It needs a function comment as
>> well.
>>
>>
>>> +static unsigned int
>>> +rtx_is_zext_p (rtx insn)
>>> +{
>>> + if (GET_CODE (insn) == AND)
>>> + {
>>> + rtx set = XEXP (insn, 0);
>>> + if (REG_P (set))
>>> + {
>>> + if (XEXP (insn, 1) == const1_rtx)
>>> + return 1;
>>> + }
>>> + else
>>> + return 0;
>>> + }
>>> +
>>> + return 0;
>>> +}
>> So my comment from the prior version stands. Testing for const1_rtx is just
>> wrong. The optimization you're trying to perform (If I understand it
>> correctly) works for many other constants and the set of constants supported
>> will vary based on the input and output modes.
>>
>> Similarly in rtx_is_zext_p.
>>
>> You still have numerous formatting issues which makes reading the patch more
>> difficult than it should be. Please review the formatting guidelines and
>> follow them. In particular please review how to indent multi-line
>> conditionals.
>>
>>
>
> Currently I support AND with const1_rtx. This is what is equivalent to zero
> extension instruction in power instruction set. When you specify many other
> constants and Could you please specify what other constants needs to be
> supported and how to determine on the Input and output modes.
>>
On top of that I support eliminating zero_extend and sign_extend wherein if
result mode of def insn not equal to source operand of zero_extend and
sign_extend.
Thanks & Regards
Ajit
>>
>>
>> You sti
>>> @@ -698,6 +777,226 @@ get_sub_rtx (rtx_insn *def_insn)
>>> return sub_rtx;
>>> }
>>> +/* Check if the def insn is ASHIFT and LSHIFTRT.
>>> + Inputs: insn for which def has to be checked.
>>> + source operand rtx.
>>> + Output: True or false if def has arithmetic
>>> + peration like ASHIFT and LSHIFTRT. */
>> This still needs work. Between the comments and code, I still don't know
>> what you're really trying to do here. I can make some guesses, but it's
>> really your job to write clear comments about what you're doing so that a
>> review or someone looking at the code in the future don't have to guess.
>>
>> It looks like you want to look at all the reaching definitions of INSN for
>> ORIG_SRC and if they are ASHIFT/LSHIFTRT do... what?
>>
>> Why are ASHIFT/LSHIFTRT interesting here? Why are you looking for them?
>>
>>
>>
>>> +
>>> +/* Find feasibility of extension elimination
>>> + across basic blocks.
>>> + Input: candiate to check the feasibility.
>>> + def_insn of candidate.
>>> + Output: Returns true or false if feasible or not. */
>> Function comments should read like complete sentences. Arguments should be
>> in all caps. There are numerous examples in ree.cc you can follow.
>>
>> There's no comments in this code which describe the properties of the
>> CFG/blocks you are looking for (domination, post-domination, whatever). It's
>> jsut a series of tests with no clear explanation of what you're looking for
>> or why any particular test exists.
>>
>> As far as I can tell you're looking at the predecessors of cand->insn and
>> make some decisions based on what you find. In some cases you return false
>> in others you return true. But there's zero indication of why you do
>> anything.
>>
>> You're checking RTL in these cases, but I suspect you really want to be
>> doing some kind of CFG property checks. But since you haven't described
>> what you're looking for, I can't make suggestions for the right queries to
>> make.
>>
>> I stopped trying to review the function at this point. It needs major work.
>> Let's start with the block/CFG properties you're looking for. We'll then
>> have to go through each section of code and repeat the process.
>>
>> In fact I might recommend pulling the code which is trying to determine CFG
>> properties into its own function. Then try to come up with a good name for
>> that function and a good function comment.
>>
>> THe next chunk of code you start looking at the properties of the candidate
>> insn. That seems like it ought to break out into its own function as well.
>>
>> THe process of refactoring, choosing good names and function comments should
>> make the overall intent of this code clearer.
>>
>>
>>
>
> I am working on this item and will incorporate them.
>
> Thanks & Regards
> Ajit
>
>> After that's done we'll review that work and perhaps go further.
>>
>> jeff
