On Sat, Jan 25, 2025 at 4:37 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Jan 24, 2025, Richard Biener <richard.guent...@gmail.com> wrote:
>
> > Hmm. I think when an original ref could trap that should be the
> > insertion point (or the original ref should post-dominate the insertion
> > point).
>
> I suppose we could do that, but... intuitively, it doesn't feel safe to
> move a nontrapping load down to combine it with a trapping load either.
>
> > It's fine when an originally may-trap reference becomes not trapping
> > (and it really cannot trap). Introducing new (may-)traps is never OK.
>
> I don't really see how a may-trap reference to a certain range of bits
> could possibly become non-trapping by merely reissuing it, so I can't
> respond intelligently to your assessment.
What I was saying that the conservative tree_could_trap_p could say 'yes'
to a certain encoding of a ref but 'no' to another if in reality the
ref can never
trap. We of course cannot (apart from bugs in tree_could_trap_p) turn
a for-sure trap into a not-trap by simply rewriting the ref.
> > So, can we split the patch to the case with the testcase at hand and
> > consider the assert and the extra tree_could_trap_p checks separately?
>
> The approved patch, with the testcase and the simplest fix, is already
> in. I've rebased the additional defensive checks and interface changes
> into the following, but I don't have any pressing need of getting them
> in: I don't know how to trigger any of the issues tested for.
>
>
> If decode_field_reference finds a load that accesses past the inner
> object's size, bail out.
>
> If loads we're attempting to combine don't have the same trapping
> status, bail out, to avoid replacing the wrong load and enabling loads
> to be moved that shouldn't.
>
> Regstrapped on x86_64-linux-gnu. Should I put this in?
>
>
> for gcc/ChangeLog
>
> PR tree-optimization/118514
> * gimple-fold.cc (decode_field_reference): Refuse to consider
> merging out-of-bounds BIT_FIELD_REFs.
> (fold_truth_andor_for_ifcombine): Check that combinable loads
> have the same trapping status.
> * tree-eh.cc (bit_field_ref_in_bounds_p): Rename to...
> (access_in_bounds_of_type_p): ... this. Change interface,
> export.
> (tree_could_trap_p): Adjust.
> * tree-eh.h (access_in_bounds_of_type_p): Declare.
> ---
> gcc/gimple-fold.cc | 16 ++++++++++------
> gcc/tree-eh.cc | 25 +++++++++++++------------
> gcc/tree-eh.h | 1 +
> 3 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 45485782cdf91..24aada4bc8fee 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -7686,10 +7686,8 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> || bs <= shiftrt
> || offset != 0
> || TREE_CODE (inner) == PLACEHOLDER_EXPR
> - /* Reject out-of-bound accesses (PR79731). */
> - || (! AGGREGATE_TYPE_P (TREE_TYPE (inner))
> - && compare_tree_int (TYPE_SIZE (TREE_TYPE (inner)),
> - bp + bs) < 0)
> + /* Reject out-of-bound accesses (PR79731, PR118514). */
> + || !access_in_bounds_of_type_p (TREE_TYPE (inner), bs, bp)
So I think we want this bit in (and it's dependences), but
> || (INTEGRAL_TYPE_P (TREE_TYPE (inner))
> && !type_has_mode_precision_p (TREE_TYPE (inner))))
> return NULL_TREE;
> @@ -8228,8 +8226,12 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> std::swap (rl_loc, rr_loc);
> }
>
> + /* Check that the loads that we're trying to combine have the same vuse and
> + same trapping status. */
> if ((ll_load && rl_load)
> - ? gimple_vuse (ll_load) != gimple_vuse (rl_load)
> + ? (gimple_vuse (ll_load) != gimple_vuse (rl_load)
> + || (tree_could_trap_p (gimple_assign_rhs1 (ll_load))
> + != tree_could_trap_p (gimple_assign_rhs1 (rl_load))))
> : (!ll_load != !rl_load))
this and
> return 0;
>
> @@ -8282,7 +8284,9 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> else if (lr_reversep != rr_reversep
> || ! operand_equal_p (lr_inner, rr_inner, 0)
> || ((lr_load && rr_load)
> - ? gimple_vuse (lr_load) != gimple_vuse (rr_load)
> + ? (gimple_vuse (lr_load) != gimple_vuse (rr_load)
> + || (tree_could_trap_p (gimple_assign_rhs1 (lr_load))
> + != tree_could_trap_p (gimple_assign_rhs1 (rr_load))))
> : (!lr_load != !rr_load)))
this not at this point (I see the assert is no longer in the patch).
So, apart from these two hunks this is OK as well.
Thanks,
Richard.
> return 0;
>
> diff --git a/gcc/tree-eh.cc b/gcc/tree-eh.cc
> index 27a4b2b5b1665..68008cea588a7 100644
> --- a/gcc/tree-eh.cc
> +++ b/gcc/tree-eh.cc
> @@ -2646,24 +2646,22 @@ range_in_array_bounds_p (tree ref)
> return true;
> }
>
> -/* Return true iff EXPR, a BIT_FIELD_REF, accesses a bit range that is known
> to
> - be in bounds for the referred operand type. */
> +/* Return true iff a BIT_FIELD_REF <(TYPE)???, SIZE, OFFSET> would access a
> bit
> + range that is known to be in bounds for TYPE. */
>
> -static bool
> -bit_field_ref_in_bounds_p (tree expr)
> +bool
> +access_in_bounds_of_type_p (tree type, poly_uint64 size, poly_uint64 offset)
> {
> - tree size_tree;
> - poly_uint64 size_max, min, wid, max;
> + tree type_size_tree;
> + poly_uint64 type_size_max, min = offset, wid = size, max;
>
> - size_tree = TYPE_SIZE (TREE_TYPE (TREE_OPERAND (expr, 0)));
> - if (!size_tree || !poly_int_tree_p (size_tree, &size_max))
> + type_size_tree = TYPE_SIZE (type);
> + if (!type_size_tree || !poly_int_tree_p (type_size_tree, &type_size_max))
> return false;
>
> - min = bit_field_offset (expr);
> - wid = bit_field_size (expr);
> max = min + wid;
> if (maybe_lt (max, min)
> - || maybe_lt (size_max, max))
> + || maybe_lt (type_size_max, max))
> return false;
>
> return true;
> @@ -2712,7 +2710,10 @@ tree_could_trap_p (tree expr)
> switch (code)
> {
> case BIT_FIELD_REF:
> - if (DECL_P (TREE_OPERAND (expr, 0)) && !bit_field_ref_in_bounds_p
> (expr))
> + if (DECL_P (TREE_OPERAND (expr, 0))
> + && !access_in_bounds_of_type_p (TREE_TYPE (TREE_OPERAND (expr, 0)),
> + bit_field_size (expr),
> + bit_field_offset (expr)))
> return true;
> /* Fall through. */
>
> diff --git a/gcc/tree-eh.h b/gcc/tree-eh.h
> index 69fe193f1b82f..1fe6de80c42e7 100644
> --- a/gcc/tree-eh.h
> +++ b/gcc/tree-eh.h
> @@ -36,6 +36,7 @@ extern void redirect_eh_dispatch_edge (geh_dispatch *,
> edge, basic_block);
> extern bool operation_could_trap_helper_p (enum tree_code, bool, bool, bool,
> bool, tree, bool *);
> extern bool operation_could_trap_p (enum tree_code, bool, bool, tree);
> +extern bool access_in_bounds_of_type_p (tree, poly_uint64, poly_uint64);
> extern bool tree_could_trap_p (tree);
> extern tree rewrite_to_non_trapping_overflow (tree);
> extern bool stmt_could_throw_p (function *, gimple *);
>
>
>
> --
> 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
