On Sun, Dec 8, 2024 at 11:17 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
> Here's yet another incremental patchlet, with the changes made in
> response to your review to v2.
>
> I'll post v3 containing it momentarily, but this incremental is supposed
> to make it easier to review.
>
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index 0d255561b1bdc..be7916e957d66 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -1477,10 +1477,6 @@ Wtemplates
> C++ ObjC++ Var(warn_templates) Warning
> Warn on primary template declaration.
>
> -Wtautological-compare
> -C ObjC C++ ObjC++ Var(warn_tautological_compare) Warning LangEnabledBy(C
> ObjC C++ ObjC++,Wall)
> -Warn if a comparison always evaluates to true or false.
> -
> Wtemplate-body
> C++ ObjC++ Var(warn_template_body) Warning Init(1)
> Diagnose errors when parsing a template.
> diff --git a/gcc/common.opt b/gcc/common.opt
> index bb226ac61e6a1..915ce5bffb4e0 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -812,6 +812,10 @@ Wsystem-headers
> Common Var(warn_system_headers) Warning
> Do not suppress warnings from system headers.
>
> +Wtautological-compare
> +Common Var(warn_tautological_compare) Warning LangEnabledBy(C ObjC C++
> ObjC++,Wall)
> +Warn if a comparison always evaluates to true or false.
> +
> Wtrampolines
> Common Var(warn_trampolines) Warning
> Warn whenever a trampoline is generated.
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 2c33cdfb1b29a..3ba4307769dd8 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -7439,6 +7439,53 @@ maybe_fold_comparisons_from_match_pd (tree type, enum
> tree_code code,
> return NULL_TREE;
> }
>
> +/* Return TRUE and set op[0] if T, following all SSA edges, is a type
> + conversion. */
> +
Like below, gimple_convert_def_p ().
> +static bool
> +gimple_fold_follow_convert (tree t, tree op[1])
> +{
> + if (TREE_CODE (t) == SSA_NAME
> + && !SSA_NAME_IS_DEFAULT_DEF (t))
> + if (gassign *def = dyn_cast <gassign *> (SSA_NAME_DEF_STMT (t)))
> + switch (gimple_assign_rhs_code (def))
> + {
> + CASE_CONVERT:
> + op[0] = gimple_assign_rhs1 (def);
> + return true;
> + case VIEW_CONVERT_EXPR:
> + op[0] = TREE_OPERAND (gimple_assign_rhs1 (def), 0);
> + return true;
> + default:
> + break;
> + }
> + return false;
> +}
> +
> +/* Return TRUE and set op[*] if T, following all SSA edges, resolves to a
> + binary expression with code CODE. */
> +
> +static bool
> +gimple_fold_binop_cst (enum tree_code code, tree t, tree op[2])
Since it doesn't actually fold - can you name it
static bool
gimple_binop_def_p (...)
and ...
> +{
> + if (TREE_CODE (t) == SSA_NAME
> + && !SSA_NAME_IS_DEFAULT_DEF (t))
> + if (gimple *def = dyn_cast <gassign *> (SSA_NAME_DEF_STMT (t)))
> + if (gimple_assign_rhs_code (def) == code)
> + {
> + tree op0 = gimple_assign_rhs1 (def);
> + tree op1 = gimple_assign_rhs2 (def);
> + if (tree_swap_operands_p (op0, op1))
> + std::swap (op0, op1);
All stmts are canonical, you shouldn't need to swap operands here.
> + if (uniform_integer_cst_p (op1))
... put this in the caller so it could be made more universal eventually?
Otherwise the incremental patch looks OK.
Richard.
> + {
> + op[0] = op0;
> + op[1] = op1;
> + return true;
> + }
> + }
> + return false;
> +}
> /* Subroutine for fold_truth_andor_1: decode a field reference.
>
> If *PEXP is a comparison reference, we return the innermost reference.
> @@ -7478,12 +7525,6 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> wide_int *pand_mask, bool *xor_p, tree *xor_cmp_op,
> gimple **load, location_t loc[4])
> {
> - /* These are from match.pd. */
> - extern bool gimple_any_convert (tree, tree *, tree (*)(tree));
> - extern bool gimple_bit_and_cst (tree, tree *, tree (*)(tree));
> - extern bool gimple_bit_xor_cst (tree, tree *, tree (*)(tree));
> - extern bool gimple_rshift_cst (tree, tree *, tree (*)(tree));
> -
> tree exp = *pexp;
> tree outer_type = 0;
> wide_int and_mask;
> @@ -7504,7 +7545,7 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> narrowing then widening casts, or vice-versa, for those that are not
> essential for the compare have already been optimized out at this
> point. */
> - if (gimple_any_convert (exp, res_ops, follow_all_ssa_edges))
> + if (gimple_fold_follow_convert (exp, res_ops))
> {
> if (!outer_type)
> {
> @@ -7515,7 +7556,7 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> }
>
> /* Recognize and save a masking operation. */
> - if (pand_mask && gimple_bit_and_cst (exp, res_ops, follow_all_ssa_edges))
> + if (pand_mask && gimple_fold_binop_cst (BIT_AND_EXPR, exp, res_ops))
> {
> loc[1] = gimple_location (SSA_NAME_DEF_STMT (exp));
> exp = res_ops[0];
> @@ -7523,7 +7564,7 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> }
>
> /* Turn (a ^ b) [!]= 0 into a [!]= b. */
> - if (xor_p && gimple_bit_xor_cst (exp, res_ops, follow_all_ssa_edges))
> + if (xor_p && gimple_fold_binop_cst (BIT_XOR_EXPR, exp, res_ops))
> {
> /* No location recorded for this one, it's entirely subsumed by the
> compare. */
> @@ -7545,7 +7586,7 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> }
>
> /* Another chance to drop conversions. */
> - if (gimple_any_convert (exp, res_ops, follow_all_ssa_edges))
> + if (gimple_fold_follow_convert (exp, res_ops))
> {
> if (!outer_type)
> {
> @@ -7556,17 +7597,19 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> }
>
> /* Take note of shifts. */
> - if (gimple_rshift_cst (exp, res_ops, follow_all_ssa_edges))
> + if (gimple_fold_binop_cst (RSHIFT_EXPR, exp, res_ops))
> {
> loc[2] = gimple_location (SSA_NAME_DEF_STMT (exp));
> exp = res_ops[0];
> + if (!tree_fits_shwi_p (res_ops[1]))
> + return NULL_TREE;
> shiftrt = tree_to_shwi (res_ops[1]);
> if (shiftrt <= 0)
> return NULL_TREE;
> }
>
> /* Yet another chance to drop conversions. */
> - if (gimple_any_convert (exp, res_ops, follow_all_ssa_edges))
> + if (gimple_fold_follow_convert (exp, res_ops))
> {
> if (!outer_type)
> {
> @@ -7712,17 +7755,26 @@ make_bit_field_load (location_t loc, tree inner, tree
> orig_inner, tree type,
> if (!point)
> return ref;
>
> - gimple_stmt_iterator gsi = gsi_for_stmt (point);
> - tree ret = force_gimple_operand_gsi (&gsi, ref,
> - true, NULL_TREE,
> - true, GSI_NEW_STMT);
> + gimple_seq stmts = NULL;
> + tree ret = force_gimple_operand (ref, &stmts, true, NULL_TREE);
> +
> /* We know the vuse is supposed to end up being the same as that at the
> original load at the insertion point, but if we don't set it, it will
> be a
> generic placeholder that only the global SSA update at the end of the
> pass
> would make equal, too late for us to use in further combinations. So go
> ahead and copy the vuse. */
> - use_operand_p use_p = gimple_vuse_op (gsi_stmt (gsi));
> - SET_USE (use_p, gimple_vuse (point));
> +
> + tree reaching_vuse = gimple_vuse (point);
> + for (gimple_stmt_iterator i = gsi_start (stmts);
> + !gsi_end_p (i); gsi_next (&i))
> + {
> + gimple *new_stmt = gsi_stmt (i);
> + if (gimple_has_mem_ops (new_stmt))
> + gimple_set_vuse (new_stmt, reaching_vuse);
> + }
> +
> + gimple_stmt_iterator gsi = gsi_for_stmt (point);
> + gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
> return ret;
> }
>
> @@ -7748,12 +7800,18 @@ build_split_load (tree /* out */ ln_arg[2],
> HOST_WIDE_INT bit_pos, bool reversep,
> gimple *point[2])
> {
> + scalar_int_mode modes[2] = { mode, mode2 };
> bitsiz[0] = GET_MODE_BITSIZE (mode);
> bitsiz[1] = GET_MODE_BITSIZE (mode2);
>
> for (int i = 0; i < 2; i++)
> {
> - tree type = lang_hooks.types.type_for_size (bitsiz[i], 1);
> + tree type = lang_hooks.types.type_for_mode (modes[i], 1);
> + if (!type)
> + {
> + type = build_nonstandard_integer_type (bitsiz[0], 1);
> + gcc_assert (type);
> + }
> bitpos[i] = bit_pos;
> ln_arg[i] = make_bit_field_load (loc, inner, orig_inner,
> type, bitsiz[i],
> @@ -7921,14 +7979,12 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> If one operand is a BIT_AND_EXPR with the constant one, treat it as if
> it were surrounded with a NE_EXPR. */
>
> - if (TREE_SIDE_EFFECTS (ll_arg) || TREE_SIDE_EFFECTS (lr_arg)
> - || TREE_SIDE_EFFECTS (rl_arg) || TREE_SIDE_EFFECTS (rr_arg))
> - return 0;
> -
> if (TREE_CODE_CLASS (lcode) != tcc_comparison
> || TREE_CODE_CLASS (rcode) != tcc_comparison)
> return 0;
>
> + /* We don't normally find TRUTH_*IF_EXPR in gimple, but these codes may be
> + given by our caller to denote conditions from different blocks. */
> switch (code)
> {
> case TRUTH_AND_EXPR:
> @@ -8196,12 +8252,9 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> make it wide enough to hold both. */
> if (l_split_load)
> lnbitsize += GET_MODE_BITSIZE (lnmode2);
> - lntype = lang_hooks.types.type_for_size (lnbitsize, 1);
> + lntype = build_nonstandard_integer_type (lnbitsize, 1);
> if (!lntype)
> - {
> - gcc_checking_assert (l_split_load);
> - lntype = build_nonstandard_integer_type (lnbitsize, 1);
> - }
> + return NULL_TREE;
> lnprec = TYPE_PRECISION (lntype);
> xll_bitpos = ll_bitpos - lnbitpos, xrl_bitpos = rl_bitpos - lnbitpos;
>
> @@ -8237,7 +8290,8 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> l_const <<= xll_bitpos;
> if ((l_const & ~ll_mask) != 0)
> {
> - warning (0, "comparison is always %d", wanted_code == NE_EXPR);
> + warning_at (lloc, OPT_Wtautological_compare,
> + "comparison is always %d", wanted_code == NE_EXPR);
>
> return constant_boolean_node (wanted_code == NE_EXPR, truth_type);
> }
> @@ -8252,7 +8306,8 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> r_const <<= xrl_bitpos;
> if ((r_const & ~rl_mask) != 0)
> {
> - warning (0, "comparison is always %d", wanted_code == NE_EXPR);
> + warning_at (rloc, OPT_Wtautological_compare,
> + "comparison is always %d", wanted_code == NE_EXPR);
>
> return constant_boolean_node (wanted_code == NE_EXPR, truth_type);
> }
> @@ -8355,12 +8410,9 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> rnbitpos = first_bit & ~ (rnbitsize - 1);
> if (r_split_load)
> rnbitsize += GET_MODE_BITSIZE (rnmode2);
> - rntype = lang_hooks.types.type_for_size (rnbitsize, 1);
> + rntype = build_nonstandard_integer_type (rnbitsize, 1);
> if (!rntype)
> - {
> - gcc_checking_assert (r_split_load);
> - rntype = build_nonstandard_integer_type (rnbitsize, 1);
> - }
> + return 0;
> rnprec = TYPE_PRECISION (rntype);
> xlr_bitpos = lr_bitpos - rnbitpos, xrr_bitpos = rr_bitpos - rnbitpos;
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 09569d1926458..2dd67b69cf1f5 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -201,17 +201,6 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> (match (maybe_bit_not @0)
> (bit_xor_cst@0 @1 @2))
>
> -/* These are used by ifcombine fold_truth_andor. */
> -(match (any_convert @0)
> - (convert @0))
> -(match (any_convert @0)
> - (view_convert @0))
> -(match (bit_and_cst @0 @1)
> - (bit_and @0 uniform_integer_cst_p@1))
> -(match (rshift_cst @0 @1)
> - (rshift @0 uniform_integer_cst_p@1)
> - (if (tree_fits_shwi_p (@1))))
> -
> #if GIMPLE
> (match (maybe_truncate @0)
> (convert @0)
>
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
