From: Ajit Agarwal <aagarwa1@linux.ibm.com>
To: Vineet Gupta <vineetg@rivosinc.com>,
gcc-patches <gcc-patches@gcc.gnu.org>
Cc: Jeff Law <jeffreyalaw@gmail.com>,
Richard Biener <richard.guenther@gmail.com>,
Segher Boessenkool <segher@kernel.crashing.org>,
Peter Bergner <bergner@linux.ibm.com>,
gnu-toolchain <gnu-toolchain@rivosinc.com>
Subject: Re: [PATCH v9 4/4] ree: Improve ree pass for rs6000 target using defined ABI interfaces
Date: Sat, 21 Oct 2023 15:44:16 +0530 [thread overview]
Message-ID: <6fa01e8c-bddf-484b-87d2-e2f728a215b5@linux.ibm.com> (raw)
In-Reply-To: <22541c92-a967-4e66-96b3-e4ad5011cd24@rivosinc.com>
Hello Vineet:
Thanks for your time and valuable comments.
On 21/10/23 5:26 am, Vineet Gupta wrote:
> On 10/19/23 23:50, Ajit Agarwal wrote:
>> Hello All:
>>
>> This version 9 of the patch uses abi interfaces to remove zero and sign extension elimination.
>> Bootstrapped and regtested on powerpc-linux-gnu.
>>
>> In this version (version 9) of the patch following review comments are incorporated.
>>
>> a) Removal of hard code zero_extend and sign_extend in abi interfaces.
>> b) Source and destination with different registers are considered.
>> c) Further enhancements.
>> d) Added sign extension elimination using abi interfaces.
>
> As has been trend in the past, I don't think all the review comments have been addressed.
> The standard practice is to reply to reviewer's email and say yay/nay explicitly to each comment. Some of my comments in [1a] are still not resolved, importantly the last two. To be fair you did reply [1b] but the comments were not addressed explicitly.
>
> [1a] https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630814.html
> [1b] https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630865.html
>
I have addressed the last 2 comments in the version 10 of the patch. Please let me know if there
is anything missing. Regarding last comments with a providing different tests if you have any suggestions please
let me know.
> Anyhow I gave this a try for RISC-V, specially after [2a][2b] as I was curious to see if this uncovers REE handling extraneous extensions which could potentially be eliminated in Expand itself, which is generally better as it happens earlier in the pipeline.
>
> [2a] 2023-10-16 8eb9cdd14218 expr: don't clear SUBREG_PROMOTED_VAR_P flag for a promoted subreg [target/111466]
> [2b] https://gcc.gnu.org/pipermail/gcc-patches/2023-October/631818.html
>
> Bad news is with the patch, we fail to even bootstrap risc-v, buckles over when building libgcc itself.
>
> The reproducer is embarrassingly simple, build with -O2:
>
> float a;
> b() { return a; }
>
> See details below....
>
>> Thanks & Regards
>> Ajit
>>
>> ree: Improve ree pass for rs6000 target using defined abi interfaces
>>
>> For rs6000 target we see redundant zero and sign extension and done
>> to improve ree pass to eliminate such redundant zero and sign extension
>> using defined ABI interfaces.
>>
>> 2023-10-20 Ajit Kumar Agarwal <aagarwa1@linux.ibm.com>
>>
>> gcc/ChangeLog:
>>
>> * ree.cc (combine_reaching_defs): Use of zero_extend and sign_extend
>> defined abi interfaces.
>> (add_removable_extension): Use of defined abi interfaces for no
>> reaching defs.
>> (abi_extension_candidate_return_reg_p): New function.
>> (abi_extension_candidate_p): New function.
>> (abi_extension_candidate_argno_p): New function.
>> (abi_handle_regs): New function.
>> (abi_target_promote_function_mode): New function.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * g++.target/powerpc/zext-elim-3.C
>> ---
>> gcc/ree.cc | 151 +++++++++++++++++-
>> .../g++.target/powerpc/zext-elim-3.C | 13 ++
>> 2 files changed, 161 insertions(+), 3 deletions(-)
>> create mode 100644 gcc/testsuite/g++.target/powerpc/zext-elim-3.C
>>
>> diff --git a/gcc/ree.cc b/gcc/ree.cc
>> index fc04249fa84..9f21f0e9907 100644
>> --- a/gcc/ree.cc
>> +++ b/gcc/ree.cc
>> @@ -514,7 +514,8 @@ get_uses (rtx_insn *insn, rtx reg)
>> if (REGNO (DF_REF_REG (def)) == REGNO (reg))
>> break;
>> - gcc_assert (def != NULL);
>> + if (def == NULL)
>> + return NULL;
>> ref_chain = DF_REF_CHAIN (def);
>> @@ -750,6 +751,124 @@ get_extended_src_reg (rtx src)
>> return src;
>> }
>> +/* Return TRUE if target mode is equal to source mode of zero_extend
>> + or sign_extend otherwise false. */
>> +
>> +static bool
>> +abi_target_promote_function_mode (machine_mode mode)
>> +{
>> + int unsignedp;
>> + machine_mode tgt_mode
>> + = targetm.calls.promote_function_mode (NULL_TREE, mode, &unsignedp,
>> + NULL_TREE, 1);
>> +
>> + if (tgt_mode == mode)
>> + return true;
>> + else
>> + return false;
>> +}
>> +
>> +/* Return TRUE if the candidate insn is zero extend and regno is
>> + a return registers. */
>> +
>> +static bool
>> +abi_extension_candidate_return_reg_p (/*rtx_insn *insn, */int regno)
>> +{
>> + if (targetm.calls.function_value_regno_p (regno))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +/* Return TRUE if reg source operand of zero_extend is argument registers
>> + and not return registers and source and destination operand are same
>> + and mode of source and destination operand are not same. */
>> +
>> +static bool
>> +abi_extension_candidate_p (rtx_insn *insn)
>> +{
>> + rtx set = single_set (insn);
>> + machine_mode dst_mode = GET_MODE (SET_DEST (set));
>> + rtx orig_src = XEXP (SET_SRC (set), 0);
>> +
>> + if (!FUNCTION_ARG_REGNO_P (REGNO (orig_src))
>> + || abi_extension_candidate_return_reg_p (/*insn,*/ REGNO (orig_src)))
>> + return false;
>> +
>> + /* Mode of destination and source should be different. */
>> + if (dst_mode == GET_MODE (orig_src))
>> + return false;
>> +
>> + machine_mode mode = GET_MODE (XEXP (SET_SRC (set), 0));
>> + bool promote_p = abi_target_promote_function_mode (mode);
>> +
>> + /* REGNO of source and destination should be same if not
>> + promoted. */
>> + if (!promote_p && REGNO (SET_DEST (set)) != REGNO (orig_src))
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +/* Return TRUE if the candidate insn is zero extend and regno is
>> + an argument registers. */
>> +
>> +static bool
>> +abi_extension_candidate_argno_p (/*rtx_code code, */int regno)
>> +{
>> + if (FUNCTION_ARG_REGNO_P (regno))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +/* Return TRUE if the candidate insn doesn't have defs and have
>> + * uses without RTX_BIN_ARITH/RTX_COMM_ARITH/RTX_UNARY rtx class. */
>> +
>> +static bool
>> +abi_handle_regs (rtx_insn *insn)
>> +{
>> + if (side_effects_p (PATTERN (insn)))
>> + return false;
>> +
>> + struct df_link *uses = get_uses (insn, SET_DEST (PATTERN (insn)));
>> +
>> + if (!uses)
>> + return false;
>> +
>> + for (df_link *use = uses; use; use = use->next)
>> + {
>> + if (!use->ref)
>> + return false;
>> +
>> + if (BLOCK_FOR_INSN (insn) != BLOCK_FOR_INSN (DF_REF_INSN (use->ref)))
>> + return false;
>> +
>> + rtx_insn *use_insn = DF_REF_INSN (use->ref);
>> +
>> + if (GET_CODE (PATTERN (use_insn)) == SET)
>> + {
>> + rtx_code code = GET_CODE (SET_SRC (PATTERN (use_insn)));
>> +
>> + if (GET_RTX_CLASS (code) == RTX_BIN_ARITH
>> + || GET_RTX_CLASS (code) == RTX_COMM_ARITH
>> + || GET_RTX_CLASS (code) == RTX_UNARY)
>> + return false;
>> + }
>> + }
>> +
>> + rtx set = single_set (insn);
>> +
>> + if (GET_CODE (SET_SRC (set)) == SIGN_EXTEND)
>> + {
>> + machine_mode mode = GET_MODE (XEXP (SET_SRC (set), 0));
>> + bool promote_p = abi_target_promote_function_mode (mode);
>> +
>> + return promote_p;
>> + }
>> + return true;
>> +}
>> +
>> /* This function goes through all reaching defs of the source
>> of the candidate for elimination (CAND) and tries to combine
>> the extension with the definition instruction. The changes
>> @@ -770,6 +889,11 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
>> state->defs_list.truncate (0);
>> state->copies_list.truncate (0);
>> + rtx orig_src = XEXP (SET_SRC (cand->expr),0);
>> +
>> + if (abi_extension_candidate_p (cand->insn)
>> + && (!get_defs (cand->insn, orig_src, NULL)))
>> + return abi_handle_regs (cand->insn);
>> outcome = make_defs_and_copies_lists (cand->insn, set_pat, state);
>> @@ -1036,6 +1160,15 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
>> }
>> }
>> + rtx insn_set = single_set (cand->insn);
>> +
>> + machine_mode mode = (GET_MODE (XEXP (SET_SRC (insn_set), 0)));
>> +
>> + bool promote_p = abi_target_promote_function_mode (mode);
>> +
>> + if (promote_p)
>> + return true;
>> +
>
> Is this early exit OK ? It skips a subsequent apply_change_group() call which could potentially fail and thus extension would be undone.
> And even for the passing case, we do want those instructions to be merged normally. So I don't understand how this change is useful at all ?
>
> FWIW for my test, w/o the patch: apply_change_group would fail (likely missing some combine pattern) and undo the merge. However w/ patch we just return/continue, keeping the merged but invalid insn which ICE in next pass cprop_hardreg as failure to recog that insn.
>
> Some more details in case you are curious:
>
> Coming into REE we have
>
> (insn 7 6 13 2 (set (reg:SI 10 a0 [136]) #DEF
> (fix:SI (reg:SF 47 fa5 [orig:138 a ] [138])))
> {fix_truncsfsi2}
>
> (insn 13 7 14 2 (set (reg/i:DI 10 a0) # extension
> (sign_extend:DI (reg:SI 10 a0 [136])))
> {extendsidi2}
>
> These are merged into unrecog insn as of now:
>
> (insn 7 6 14 2 (set (reg:DI 10 a0)
> (sign_extend:DI (fix:SI (reg:SF 47 fa5 [orig:138 a ] [138]))))
>
Thanks for your input on this. I have addressed that in the version 10 of the patch please review.
Thanks & Regards
Ajit
> Thx,
> -Vineet
>
>> if (merge_successful)
>> {
>> /* Commit the changes here if possible
>> @@ -1112,6 +1245,14 @@ add_removable_extension (const_rtx expr, rtx_insn *insn,
>> rtx reg = XEXP (src, 0);
>> struct df_link *defs, *def;
>> ext_cand *cand;
>> + defs = get_defs (insn, reg, NULL);
>> +
>> + if (!defs && abi_extension_candidate_argno_p (/*code,*/ REGNO (reg)))
>> + {
>> + ext_cand e = {expr, code, mode, insn};
>> + insn_list->safe_push (e);
>> + return;
>> + }
>> /* Zero-extension of an undefined value is partly defined (it's
>> completely undefined for sign-extension, though). So if there exists
>> @@ -1131,7 +1272,6 @@ add_removable_extension (const_rtx expr, rtx_insn *insn,
>> }
>> /* Second, make sure we can get all the reaching definitions. */
>> - defs = get_defs (insn, reg, NULL);
>> if (!defs)
>> {
>> if (dump_file)
>> @@ -1321,7 +1461,8 @@ find_and_remove_re (void)
>> && (REGNO (SET_DEST (set)) != REGNO (XEXP (SET_SRC (set), 0))))
>> {
>> reinsn_copy_list.safe_push (curr_cand->insn);
>> - reinsn_copy_list.safe_push (state.defs_list[0]);
>> + if (state.defs_list.length () != 0)
>> + reinsn_copy_list.safe_push (state.defs_list[0]);
>> }
>> reinsn_del_list.safe_push (curr_cand->insn);
>> state.modified[INSN_UID (curr_cand->insn)].deleted = 1;
>> @@ -1345,6 +1486,10 @@ find_and_remove_re (void)
>> for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
>> {
>> rtx_insn *curr_insn = reinsn_copy_list[i];
>> +
>> + if ((i+1) >= reinsn_copy_list.length ())
>> + continue;
>> +
>> rtx_insn *def_insn = reinsn_copy_list[i + 1];
>> /* Use the mode of the destination of the defining insn
>> diff --git a/gcc/testsuite/g++.target/powerpc/zext-elim-3.C b/gcc/testsuite/g++.target/powerpc/zext-elim-3.C
>> new file mode 100644
>> index 00000000000..5a050df06ff
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.target/powerpc/zext-elim-3.C
>> @@ -0,0 +1,13 @@
>> +/* { dg-options "-mcpu=power9 -O2" } */
>> +
>> +void *memset(void *b, int c, unsigned long len)
>> +{
>> + unsigned long i;
>> +
>> + for (i = 0; i < len; i++)
>> + ((unsigned char *)b)[i] = c;
>> +
>> + return b;
>> +}
>> +
>> +/* { dg-final { scan-assembler-not "\mrlwinm\M" } } */
>
next prev parent reply other threads:[~2023-10-21 10:14 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-20 6:50 Ajit Agarwal
2023-10-20 23:56 ` Vineet Gupta
2023-10-21 10:14 ` Ajit Agarwal [this message]
2023-10-21 19:26 ` rep.dot.nop
2023-10-23 6:46 ` Ajit Agarwal
2023-10-23 14:10 ` Bernhard Reutner-Fischer
2023-10-24 7:36 ` Ajit Agarwal
2023-10-24 20:36 ` rep.dot.nop
2023-10-24 20:49 ` Vineet Gupta
2023-10-25 11:11 ` Ajit Agarwal
2023-10-27 17:16 ` Bernhard Reutner-Fischer
2023-10-27 22:39 ` Vineet Gupta
2023-10-28 10:26 ` Ajit Agarwal
2023-10-29 10:49 ` Ajit Agarwal
2023-10-28 10:25 ` Ajit Agarwal
2023-10-29 10:48 ` Ajit Agarwal
2023-10-25 11:08 ` Ajit Agarwal
2023-10-23 18:32 ` Vineet Gupta
2023-10-24 7:40 ` Ajit Agarwal
2023-10-24 9:36 ` Ajit Agarwal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6fa01e8c-bddf-484b-87d2-e2f728a215b5@linux.ibm.com \
--to=aagarwa1@linux.ibm.com \
--cc=bergner@linux.ibm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=gnu-toolchain@rivosinc.com \
--cc=jeffreyalaw@gmail.com \
--cc=richard.guenther@gmail.com \
--cc=segher@kernel.crashing.org \
--cc=vineetg@rivosinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).