> Am 17.12.2024 um 14:30 schrieb Alexandre Oliva <ol...@adacore.com>:
>
>
> ACATS-4 ca11d02 exposed an error in the logic for recognizing and
> identifying the inner object in decode_field_ref: a view-converting
> load, inserted in a previous successful field merging operation, was
> recognized by gimple_convert_def_p within decode_field_reference, and
> as a result we took its operand as the expression, and failed to take
> note of the load location.
>
> Without that load, we couldn't compare vuses, and then we ended up
> inserting a wider load before relevant parts of the object were
> initialized.
>
> This patch makes gimple_convert_def_p recognize loads only when
> requested, and requires that either both or neither parts of a
> potentially merged operand have associated loads.
>
> As a bonus, it enables additional optimizations by swapping the
> operands of the second compare when that makes left-hand operands
> of both compares match.
>
> Regstrapped on x86_64-linux-gnu and on ppc64-linux-gnu, along with 3
> other ifcombine patches. Ok to install?
Ok
Richard
>
> for gcc/ChangeLog
>
> * gimple-fold.cc (gimple_convert_def_p): Reject load stmts
> unless requested.
> (decode_field_reference): Accept a converting load at the last
> conversion matcher, subsuming the load identification.
> (fold_truth_andor_for_ifcombine): Refuse to merge operands
> when only one of them has an associated load stmt. Swap
> operands of one of the compares if that helps them match.
>
> for gcc/testsuite/ChangeLog
>
> * gcc.dg/field-merge-13.c: New.
> ---
> gcc/gimple-fold.cc | 93 ++++++++++++++++++++++++---------
> gcc/testsuite/gcc.dg/field-merge-13.c | 93 +++++++++++++++++++++++++++++++++
> 2 files changed, 162 insertions(+), 24 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/field-merge-13.c
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 6c11654a2c65b..92f02ddd77408 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -7440,26 +7440,42 @@ maybe_fold_comparisons_from_match_pd (tree type, enum
> tree_code code,
> }
>
> /* Return TRUE and set op[0] if T, following all SSA edges, is a type
> - conversion. */
> + conversion. Reject loads if LOAD is NULL, otherwise set *LOAD if a
> + converting load is found. */
>
> static bool
> -gimple_convert_def_p (tree t, tree op[1])
> +gimple_convert_def_p (tree t, tree op[1], gimple **load = NULL)
> {
> + bool ret = false;
> +
> 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;
> + {
> + bool load_p = gimple_assign_load_p (def);
> + if (load_p && !load)
> + return false;
> + switch (gimple_assign_rhs_code (def))
> + {
> + CASE_CONVERT:
> + op[0] = gimple_assign_rhs1 (def);
> + ret = true;
> + break;
> +
> + case VIEW_CONVERT_EXPR:
> + op[0] = TREE_OPERAND (gimple_assign_rhs1 (def), 0);
> + ret = true;
> + break;
> +
> + default:
> + break;
> + }
> +
> + if (ret && load_p)
> + *load = def;
> + }
> +
> + return ret;
> }
>
> /* Return TRUE and set op[*] if T, following all SSA edges, resolves to a
> @@ -7604,19 +7620,23 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> return NULL_TREE;
> }
>
> - /* Yet another chance to drop conversions. */
> - if (gimple_convert_def_p (exp, res_ops))
> + /* Yet another chance to drop conversions. This one is allowed to
> + match a converting load, subsuming the load identification block
> + below. */
> + if (gimple_convert_def_p (exp, res_ops, load))
> {
> if (!outer_type)
> {
> outer_type = TREE_TYPE (exp);
> loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
> }
> + if (*load)
> + loc[3] = gimple_location (*load);
> exp = res_ops[0];
> }
>
> /* Identify the load, if there is one. */
> - if (TREE_CODE (exp) == SSA_NAME
> + if (!(*load) && TREE_CODE (exp) == SSA_NAME
> && !SSA_NAME_IS_DEFAULT_DEF (exp))
> {
> gimple *def = SSA_NAME_DEF_STMT (exp);
> @@ -8056,13 +8076,37 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> /* It must be true that the inner operation on the lhs of each
> comparison must be the same if we are to be able to do anything.
> Then see if we have constants. If not, the same must be true for
> - the rhs's. */
> + the rhs's. If one is a load and the other isn't, we have to be
> + conservative and avoid the optimization, otherwise we could get
> + SRAed fields wrong. */
> if (volatilep
> || ll_reversep != rl_reversep
> - || ll_inner == 0 || rl_inner == 0
> - || ! operand_equal_p (ll_inner, rl_inner, 0)
> - || (ll_load && rl_load
> - && gimple_vuse (ll_load) != gimple_vuse (rl_load)))
> + || ll_inner == 0 || rl_inner == 0)
> + return 0;
> +
> + if (! operand_equal_p (ll_inner, rl_inner, 0))
> + {
> + /* Try swapping the operands. */
> + if (ll_reversep != rr_reversep
> + || !rr_inner
> + || !operand_equal_p (ll_inner, rr_inner, 0))
> + return 0;
> +
> + rcode = swap_tree_comparison (rcode);
> + std::swap (rl_arg, rr_arg);
> + std::swap (rl_inner, rr_inner);
> + std::swap (rl_bitsize, rr_bitsize);
> + std::swap (rl_bitpos, rr_bitpos);
> + std::swap (rl_unsignedp, rr_unsignedp);
> + std::swap (rl_reversep, rr_reversep);
> + std::swap (rl_and_mask, rr_and_mask);
> + std::swap (rl_load, rr_load);
> + std::swap (rl_loc, rr_loc);
> + }
> +
> + if ((ll_load && rl_load)
> + ? gimple_vuse (ll_load) != gimple_vuse (rl_load)
> + : (!ll_load != !rl_load))
> return 0;
>
> if (TREE_CODE (lr_arg) == INTEGER_CST
> @@ -8083,8 +8127,9 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> else if (lr_reversep != rr_reversep
> || lr_inner == 0 || rr_inner == 0
> || ! operand_equal_p (lr_inner, rr_inner, 0)
> - || (lr_load && rr_load
> - && gimple_vuse (lr_load) != gimple_vuse (rr_load)))
> + || ((lr_load && rr_load)
> + ? gimple_vuse (lr_load) != gimple_vuse (rr_load)
> + : (!lr_load != !rr_load)))
> return 0;
>
> /* If we found sign tests, finish turning them into bit tests. */
> diff --git a/gcc/testsuite/gcc.dg/field-merge-13.c
> b/gcc/testsuite/gcc.dg/field-merge-13.c
> new file mode 100644
> index 0000000000000..7e4f4c499347f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/field-merge-13.c
> @@ -0,0 +1,93 @@
> +/* { dg-do run } */
> +/* { dg-options "-O -fdump-tree-ifcombine-details" } */
> +
> +/* Check that we optimize swapped compares, and that we don't load from
> objects
> + before they're fully initialized. */
> +
> +struct pair {
> + signed char a;
> + signed char b;
> +} __attribute__ ((aligned (2)));
> +
> +#define make_pair(a,b) { a, b }
> +
> +struct s {
> + struct pair c;
> + struct pair d;
> +} __attribute__ ((aligned (4)));
> +
> +struct pair cp = make_pair (127, -1);
> +struct pair dp = make_pair (42, 0xf1);
> +
> +struct pair cq = make_pair (127, -1);
> +struct pair dq = make_pair (42, 0xf1);
> +
> +struct pair cr = make_pair (-127, -1);
> +struct pair dr = make_pair (42, 0xff);
> +
> +static __attribute__ ((noinline, noclone, noipa))
> +struct pair copy_pair (struct pair c)
> +{
> + return c;
> +}
> +
> +static inline
> +struct s
> +make_s (struct pair c, struct pair d)
> +{
> + struct s r;
> + r.c = copy_pair (c);
> + r.d = copy_pair (d);
> + return r;
> +}
> +
> +void f (void) {
> + struct s p = make_s (cp, dp);
> + struct s q = make_s (cq, dr);
> +
> + if (0
> + || p.c.a != q.c.a
> + || q.c.b != p.c.b
> + || p.d.b != q.d.b
> + || q.d.a != p.d.a
> + )
> + return;
> + __builtin_abort ();
> +}
> +
> +void g (void) {
> + struct s p = make_s (cp, dp);
> + struct s q = make_s (cr, dq);
> +
> + if (0
> + || p.c.a != q.c.a
> + || q.c.b != p.c.b
> + || p.d.b != q.d.b
> + || q.d.a != p.d.a
> + )
> + return;
> + __builtin_abort ();
> +}
> +
> +void h (void) {
> + struct s p = make_s (cp, dp);
> + struct s q = make_s (cq, dq);
> +
> + if (0
> + || p.c.a != q.c.a
> + || q.c.b != p.c.b
> + || p.d.b != q.d.b
> + || q.d.a != p.d.a
> + )
> + __builtin_abort ();
> + return;
> +}
> +
> +int main () {
> + f ();
> + g ();
> + h ();
> + return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "optimizing" 9 "ifcombine" } } */
>
> --
> 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
