On Thu, 30 Mar 2023 17:30:43 +0530
Ajit Agarwal via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> Hello All:
Just some nits.
And this seems to be an incremental diff ontop of your v2.
You might want check for coding-style errors by running:
contrib/check_GNU_style.py /tmp/ree-v2bis.patch
>
> This patch added enhancement to extimination elimination for rs6000 target.
I think REE means redundant extension elimination.
> gcc/ChangeLog:
>
> * ree.cc(is_feasible_elim_across_basic_blocks):
We often use the lispy _p suffix for predicates.
Maybe eliminate_across_bbs_p would be shorter.
> Add checks to enable extension elimination..
> * ree.cc(combine_reaching_defs): Add checks to enable
> extension elimination.
> ---
> gcc/ree.cc | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 116 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/ree.cc b/gcc/ree.cc
> index d05d37f9a23..7e4cce5cee4 100644
> --- a/gcc/ree.cc
> +++ b/gcc/ree.cc
> @@ -253,6 +253,56 @@ struct ext_cand
>
> static int max_insn_uid;
>
> +/* Get all the reaching definitions of an instruction. The definitions are
> + desired for REG used in INSN. Return the definition list or NULL if a
> + definition is missing. If DEST is non-NULL, additionally push the INSN
> + of the definitions onto DEST. */
> +
> +static struct df_link *
> +get_defs (rtx_insn *insn, rtx reg, vec<rtx_insn *> *dest)
> +{
> + df_ref use;
> + struct df_link *ref_chain, *ref_link;
> +
> + FOR_EACH_INSN_USE (use, insn)
> + {
> + if (GET_CODE (DF_REF_REG (use)) == SUBREG)
> + return NULL;
> + if (REGNO (DF_REF_REG (use)) == REGNO (reg))
> + break;
> + }
> +
> + if (use == NULL) return NULL;
Missing newline before return.
> +
> + gcc_assert (use != NULL);
Either the assert or the if but both is a bit much.
> +
> + ref_chain = DF_REF_CHAIN (use);
> +
> + for (ref_link = ref_chain; ref_link; ref_link = ref_link->next)
> + {
> + /* Problem getting some definition for this instruction. */
> + if (ref_link->ref == NULL)
> + return NULL;
> + if (DF_REF_INSN_INFO (ref_link->ref) == NULL)
> + return NULL;
> + /* As global regs are assumed to be defined at each function call
> + dataflow can report a call_insn as being a definition of REG.
> + But we can't do anything with that in this pass so proceed only
> + if the instruction really sets REG in a way that can be deduced
> + from the RTL structure. */
> + if (global_regs[REGNO (reg)]
> + && !set_of (reg, DF_REF_INSN (ref_link->ref)))
> + return NULL;
> + }
> +
> + if (dest)
> + for (ref_link = ref_chain; ref_link; ref_link = ref_link->next)
> + dest->safe_push (DF_REF_INSN (ref_link->ref));
> +
> + return ref_chain;
> +}
> +
> +
> /* Identify instruction AND with identical zero extension. */
>
> static unsigned int
> @@ -393,6 +443,7 @@ combine_set_extension (ext_cand *cand, rtx_insn
> *curr_insn, rtx *orig_set)
> machine_mode orig_mode = GET_MODE (SET_DEST (*orig_set));
> rtx new_set = NULL_RTX;
> rtx cand_pat = single_set (cand->insn);
> +
superfluous whitespace change.
> if (insn_is_zext_p(cand->insn)
> && CONST_INT_P (orig_src) && INTVAL (orig_src) != 0)
> return false;
> @@ -401,6 +452,7 @@ combine_set_extension (ext_cand *cand, rtx_insn
> *curr_insn, rtx *orig_set)
> then we need to change the original load to reference the destination
> of the extension. Then we need to emit a copy from that destination
> to the original destination of the load. */
> + // print_rtl_single(stdout, cand_pat);
Please remove that debug comment.
> rtx new_reg;
> bool copy_needed
> = (REGNO (SET_DEST (cand_pat)) != REGNO (XEXP (SET_SRC (cand_pat), 0)));
> @@ -547,6 +599,7 @@ transform_ifelse (ext_cand *cand, rtx_insn *def_insn)
> return false;
> }
>
> +#if 0
> /* Get all the reaching definitions of an instruction. The definitions are
> desired for REG used in INSN. Return the definition list or NULL if a
> definition is missing. If DEST is non-NULL, additionally push the INSN
> @@ -593,7 +646,7 @@ get_defs (rtx_insn *insn, rtx reg, vec<rtx_insn *> *dest)
>
> return ref_chain;
> }
> -
> +#endif
Why did you move get_defs?
> /* Get all the reaching uses of an instruction. The uses are desired for REG
> set in INSN. Return use list or NULL if a use is missing or irregular.
> */
>
> @@ -848,6 +901,36 @@ is_feasible_elim_across_basic_blocks (ext_cand *cand,
> && REGNO (XEXP (PATTERN (BB_END (bb)), 0)) != REGNO (SET_DEST
> (cand->expr)))
> return false;
>
> + if (cand->code == ZERO_EXTEND
> + && GET_CODE ((PATTERN (def_insn))) == PARALLEL)
> + return false;
Excess braces above.
> +
> + if (cand->code == ZERO_EXTEND
> + && GET_CODE ((PATTERN (def_insn))) == SET
Excess braces above.
> + && GET_CODE (SET_SRC (PATTERN (def_insn))) != XOR)
> + return false;
> +
> + if (cand->code == ZERO_EXTEND
> + && GET_CODE ((PATTERN (def_insn))) == SET
> + && GET_CODE (SET_SRC (PATTERN (def_insn))) == XOR)
> + {
> + vec<rtx_insn *> *dest = XCNEWVEC (vec<rtx_insn *>, 4);
> + if (!get_defs (def_insn, XEXP (SET_SRC (PATTERN(def_insn)), 0), dest))
> + return false;
missing space between PATTERN and the brace
This leaks dest.
The above looks a bit redundant.
I'd have a single test for cand->code == ZERO_EXTEND and then
rtx def = PATTERN (def_insn);
if (def == PARALLEL)
return false;
if (def == SET)
{
rtx src = SET_SRC (def);
if (GET_CODE (src) != XOR)
return false;
/* maybe use an auto_vec here */
...
> +
> + int i;
> + rtx_insn *def_insn;
> +
> + FOR_EACH_VEC_ELT (*dest, i, def_insn)
> + {
> + if ((GET_CODE (PATTERN (def_insn)) == SET
> + && GET_CODE (SET_SRC (PATTERN (def_insn))) == ASHIFT)
> + || GET_CODE (PATTERN (def_insn)) == PARALLEL)
leaks dest.
> + return false;
> + }
> + XDELETEVEC (dest);
> + }
> +
> return true;
> }
>
> @@ -873,7 +956,8 @@ merge_def_and_ext (ext_cand *cand, rtx_insn *def_insn,
> ext_state *state)
>
> if (!feasible) return false;
>
> - if (((!copy_needed && (insn_is_zext_p (cand->insn))
> + if (((!copy_needed && (insn_is_zext_p (cand->insn)
> + || (cand->code == ZERO_EXTEND && ext_src_mode == QImode))
> && (GET_MODE (SET_DEST (*sub_rtx)) != ext_src_mode
> && state->modified[INSN_UID (def_insn)].kind == EXT_MODIFIED_NONE))
> || ((state->modified[INSN_UID (def_insn)].kind
> @@ -1211,15 +1295,22 @@ combine_reaching_defs (ext_cand *cand, const_rtx
> set_pat, ext_state *state)
> definitions could be merged. */
> if (apply_change_group ())
> {
> - if (dump_file)
> - fprintf (dump_file, "All merges were successful.\n");
> + if (state->modified_list.length() == 0) return false;
Missing space after length before the opening brace, and missing
newline before return.
> +
> + if (cand->code == ZERO_EXTEND
> + && GET_CODE (PATTERN (state->modified_list[0])) == SET
> + && GET_CODE (SET_SRC (PATTERN (state->modified_list[0]))) != XOR)
> + return false;
> +
> + if (dump_file)
> + fprintf (dump_file, "All merges were successful.\n");
>
> FOR_EACH_VEC_ELT (state->modified_list, i, def_insn)
> {
> ext_modified *modified = &state->modified[INSN_UID (def_insn)];
> if (modified->kind == EXT_MODIFIED_NONE)
> modified->kind = (cand->code == ZERO_EXTEND ? EXT_MODIFIED_ZEXT
> - :
> EXT_MODIFIED_SEXT);
> + :
> EXT_MODIFIED_SEXT);
>
> if (copy_needed)
> modified->do_not_reextend = 1;
> @@ -1228,6 +1319,26 @@ combine_reaching_defs (ext_cand *cand, const_rtx
> set_pat, ext_state *state)
> }
> else
> {
> + if (state->modified_list.length() == 0) return false;
same comment as above
> +
> + if (cand->code == ZERO_EXTEND
> + && GET_CODE (PATTERN(state->modified_list[0])) == SET
> + && GET_CODE (SET_SRC (PATTERN (state->modified_list[0]))) !=
> XOR)
> + return false;
Missing space before '('
> +
> + if (cand->code == ZERO_EXTEND)
> + {
> + FOR_EACH_VEC_ELT (state->modified_list, i, def_insn)
> + {
> + ext_modified *modified = &state->modified[INSN_UID
> (def_insn)];
> + if (modified->kind == EXT_MODIFIED_NONE)
> + modified->kind = (cand->code == ZERO_EXTEND ?
> EXT_MODIFIED_ZEXT
> + :
> EXT_MODIFIED_SEXT);
The whole loop is guarded by code == ZERO_EXTEND, so the rhs condition
above is redundant.
thanks,
> +
> + modified->do_not_reextend = 1;
> + }
> + return true;
> + }
> /* Changes need not be cancelled explicitly as apply_change_group
> does it. Print list of definitions in the dump_file for debug
> purposes. This extension cannot be deleted. */
