On Tue, Apr 04, 2023 at 05:02:35PM +0530, Ajit Agarwal via Gcc-patches wrote:
> This patch eliminates unnecessary redundant extension within basic and across
> basic blocks. For rs6000 target we see
> redundant zero and sign extension and done to improve ree pass to eliminate
> such redundant zero and sign extension.
Just random comments, will defer the rest to whomever reviews it for stage1.
> testsuite/g++.target/powerpc/zext-elim.C: New testcase.
> testsuite/g++.target/powerpc/sext-elim.C: New testcase.
Missing * and space before the filenames, plus testsuite has a separate
ChangeLog, so it should be
gcc/testsuite/ChangeLog
* g++.target/powerpc/zext-elim.C: New testcase.
etc.
> --- a/gcc/common/config/rs6000/rs6000-common.cc
> +++ b/gcc/common/config/rs6000/rs6000-common.cc
> @@ -30,6 +30,8 @@
> /* Implement TARGET_OPTION_OPTIMIZATION_TABLE. */
> static const struct default_options rs6000_option_optimization_table[] =
> {
> + /* Enable -free for zero extension and sign extension elimination.*/
> + { OPT_LEVELS_2_PLUS, OPT_free, NULL, 1 },
I believe the options should be sorted by the OPT_LEVEL* they are given.
> /* Split multi-word types early. */
> { OPT_LEVELS_ALL, OPT_fsplit_wide_types_early, NULL, 1 },
> /* Enable -fsched-pressure for first pass instruction scheduling. */
> @@ -38,11 +40,9 @@ static const struct default_options
> rs6000_option_optimization_table[] =
> loops at -O2 and above by default. */
> { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 },
> { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_only_small_loops, NULL, 1 },
> -
> /* -frename-registers leads to non-optimal codegen and performance
> on rs6000, turn it off by default. */
> { OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 },
> -
> /* Double growth factor to counter reduced min jump length. */
> { OPT_LEVELS_ALL, OPT__param_max_grow_copy_bb_insns_, NULL, 16 },
> { OPT_LEVELS_NONE, 0, NULL, 0 }
Why?
> + rtx set = XEXP (SET_SRC(body), 0);
Space missing before (body
The indentation is also incorrect, it should be always in multiplies
of 2, the above is 5 columns.
> +
> + if (REG_P(set) && GET_MODE(SET_DEST(body))
> + == GET_MODE(set))
Similarly. Furthermore, the formatting is wrong. In this case, there is no
reason why
if (REG_P (set) && GET_MODE (SET_DEST (body)) == GET_MODE (set))
wouldn't fit on one line, but if it would be longer, either one would use
if (whatever
&& whatever_else)
or
if (whatever && (whatever_else
== something))
or similar.
> + && GET_CODE (SET_DEST (expr)) == REG
Use REG_P (SET_DEST (expr))
> + && GET_CODE (SET_SRC (expr)) == IF_THEN_ELSE
> + && GET_CODE (XEXP (SET_SRC (expr), 1)) == REG
> + && GET_CODE (XEXP (SET_SRC (expr), 2)) == REG)
Similarly.
> + {
> + *reg1 = XEXP (SET_SRC (expr), 1);
> + *reg2 = XEXP (SET_SRC (expr), 2);
> + return true;
> + }
> @@ -319,9 +440,8 @@ combine_set_extension (ext_cand *cand, rtx_insn
> *curr_insn, rtx *orig_set)
> {
> rtx orig_src = SET_SRC (*orig_set);
> machine_mode orig_mode = GET_MODE (SET_DEST (*orig_set));
> - rtx new_set;
> + rtx new_set = NULL_RTX;
> rtx cand_pat = single_set (cand->insn);
> -
Why this random whitespace change?
> @@ -734,8 +1006,7 @@ merge_def_and_ext (ext_cand *cand, rtx_insn *def_insn,
> ext_state *state)
> return true;
> }
> }
> -
> - return false;
> + return false;
> }
The old code was properly formatted, this one isn't.
> /* Given SRC, which should be one or more extensions of a REG, strip
> @@ -744,7 +1015,8 @@ merge_def_and_ext (ext_cand *cand, rtx_insn *def_insn,
> ext_state *state)
> static inline rtx
> get_extended_src_reg (rtx src)
> {
> - while (GET_CODE (src) == SIGN_EXTEND || GET_CODE (src) == ZERO_EXTEND)
> + while (GET_CODE (src) == SIGN_EXTEND || GET_CODE (src) == ZERO_EXTEND
> + || insn_is_zext_p(src))
If a condition doesn't fit into one line, then the &&/|| terms should be
split into one term on one line, so
while (GET_CODE (src) == SIGN_EXTEND
|| GET_CODE (src) == ZERO_EXTEND
|| insn_is_zext_p (src))
and note again missing space.
> src = XEXP (src, 0);
> gcc_assert (REG_P (src));
> return src;
> @@ -767,10 +1039,48 @@ combine_reaching_defs (ext_cand *cand, const_rtx
> set_pat, ext_state *state)
> int i;
> int defs_ix;
> bool outcome;
> -
Again, why? Don't do this.
> state->defs_list.truncate (0);
> state->copies_list.truncate (0);
>
> + if (cand->code == ZERO_EXTEND)
> + {
> + rtx orig_src = XEXP (SET_SRC (cand->expr),0);
> + if (!get_defs (cand->insn, orig_src, NULL))
> + {
> + if (GET_MODE (orig_src) == QImode
> + && FUNCTION_ARG_REGNO_P (REGNO (orig_src))
> + && !FUNCTION_VALUE_REGNO_P (REGNO (orig_src)))
> + {
> + if (side_effects_p (PATTERN (cand->insn)))
> + return false;
> +
> + struct df_link *uses
> + = get_uses (cand->insn, SET_DEST (PATTERN (cand->insn)));
> +
> + if (!uses) return false;
> +
> + for (df_link *use = uses; use; use = use->next)
> + {
> + if (BLOCK_FOR_INSN (cand->insn)
> + != BLOCK_FOR_INSN (DF_REF_INSN (use->ref)))
> + return false;
> +
> + rtx_insn *insn = DF_REF_INSN (use->ref);
> +
> + if (GET_CODE (PATTERN (insn)) == SET)
> + {
> + rtx_code code = GET_CODE (SET_SRC (PATTERN (insn)));
> + if (GET_RTX_CLASS (code) == RTX_BIN_ARITH
> + || GET_RTX_CLASS (code) == RTX_COMM_ARITH
> + || GET_RTX_CLASS (code) == RTX_UNARY)
> + return false;
> + }
> + }
> + return true;
> + }
> + }
> + }
Again, indent by 5 spaces can't be right.
> - modified->kind = (cand->code == ZERO_EXTEND ? EXT_MODIFIED_ZEXT
> - :
> EXT_MODIFIED_SEXT);
> + modified->kind = (cand->code == ZERO_EXTEND ? EXT_MODIFIED_ZEXT
> + :
> EXT_MODIFIED_SEXT);
Why? Old code was properly formatted, this one isn't.
> @@ -1368,9 +1730,11 @@ find_and_remove_re (void)
> reinsn_list.release ();
> XDELETEVEC (state.modified);
>
> - if (dump_file && num_re_opportunities > 0)
> + if (dump_file && num_re_opportunities > 0) {
> fprintf (dump_file, "Elimination opportunities = %d realized = %d\n",
> num_re_opportunities, num_realized);
> + }
> +
> }
Again, why? This hunk turns something properly formatted into something
with multiple formatting issues in it.
Jakub
