On Thu, Jan 23, 2025 at 6:58 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Jan 22, 2025, Alexandre Oliva <ol...@adacore.com> wrote:
>
> > I have another patch coming up that doesn't raise concerns for me, so
> > I'll hold off from installing the above, in case you also prefer the
> > other one.
>
> Unlike other access patterns, BIT_FIELD_REFs aren't regarded as
> possibly-trapping out of referencing out-of-bounds bits.
>
> So, if decode_field_reference finds a load that could trap, but whose
> inner object can't, bail out if it accesses past the inner object's
> size.
>
> This may drop some optimizations in VLAs, that could be safe if we
> managed to reuse the same pre-existing load for both compares, but
> those get optimized later (as in the new testcase). The cases of most
> interest are those that merge separate fields, and they necessarily
> involve new BIT_FIELD_REFs loads.
>
> Regstrapped on x86_64-linux-gnu. Ok to install instead of the other
> one?
>
>
> for gcc/ChangeLog
>
> PR tree-optimization/118514
> * gimple-fold.cc (decode_field_reference): Refuse to consider
> BIT_FIELD_REF of non-trapping object if the original load
> could trap for being out-of-bounds.
> (make_bit_field_ref): Check that replacement loads could trap
> as much as the original loads.
> (fold_truth_andor_for_ifcombine): Rearrange testing of
> reversep, and note that signbit needs not be concerned with
> it. Check that combinable loads have the same trapping
> status.
> * tree-eh.cc (bit_field_ref_in_bounds_p): New.
> (tree_could_trap_p): Call it on DECLs.
> * tree-eh.h (bit_field_ref_in_bounds_p): Declare.
>
> for gcc/testsuite/ChangeLog
>
> PR tree-optimization/118514
> * gcc.dg/field-merge-23.c: New.
> ---
> gcc/gimple-fold.cc | 27 +++++++++++++++++++++------
> gcc/testsuite/gcc.dg/field-merge-23.c | 19 +++++++++++++++++++
> gcc/tree-eh.cc | 30 +++++++++++++++++++++++++++++-
> gcc/tree-eh.h | 1 +
> 4 files changed, 70 insertions(+), 7 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/field-merge-23.c
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index da3f505c3fcac..cd9d350349186 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -7686,10 +7686,14 @@ 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). BIT_FIELD_REFs aren't
> flagged
> + as tree_could_trap_p just because of out-of-range bits, so don't even
> + try to optimize loads that could trap if they access out-of-range
> bits
> + of an object that could not trap (PR118514). */
> + || ((! AGGREGATE_TYPE_P (TREE_TYPE (inner))
> + || (load && tree_could_trap_p (gimple_assign_rhs1 (load))
> + && !tree_could_trap_p (inner)))
> + && !bit_field_ref_in_bounds_p (inner, bs, bp))
The new bit_field_ref_in_bounds_p is really an
"access in bound of type" now, and IMO that's something
we should simply always guarantee here - the exception
might be again when we can't prove this due to the TYPE_SIZE
of 'inner' being not (poly-)constant, but those cases should always
end up trapping.
That said, it'd be a lot clearer if this simply read
|| !access_in_bounds_of_type_p (TREE_TYPE (inner), bs, bp)
without all the other weird conditions.
> || (INTEGRAL_TYPE_P (TREE_TYPE (inner))
> && !type_has_mode_precision_p (TREE_TYPE (inner))))
> return NULL_TREE;
> @@ -7859,6 +7863,11 @@ make_bit_field_load (location_t loc, tree inner, tree
> orig_inner, tree type,
> gimple *new_stmt = gsi_stmt (i);
> if (gimple_has_mem_ops (new_stmt))
> gimple_set_vuse (new_stmt, reaching_vuse);
> + gcc_checking_assert (! (gimple_assign_load_p (point)
> + && gimple_assign_load_p (new_stmt))
> + || (tree_could_trap_p (gimple_assign_rhs1 (point))
> + == tree_could_trap_p (gimple_assign_rhs1
> + (new_stmt))));
> }
>
> gimple_stmt_iterator gsi = gsi_for_stmt (point);
> @@ -8223,8 +8232,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))))
So that's to make the above added assert happy, right? At least I don't see any
other good reason to enforce this?
> : (!ll_load != !rl_load))
> return 0;
>
> @@ -8277,7 +8290,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)))
> return 0;
>
> diff --git a/gcc/testsuite/gcc.dg/field-merge-23.c
> b/gcc/testsuite/gcc.dg/field-merge-23.c
> new file mode 100644
> index 0000000000000..96604d43c9dec
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/field-merge-23.c
> @@ -0,0 +1,19 @@
> +/* { dg-do run } */
> +/* { dg-options "-O" } */
> +
> +/* PR tree-optimization/118514 */
> +
> +/* Check that we don't create BIT_FIELD_REFs that could trap, because they
> are
> + assumed not to trap and could be pulled out of loops. */
> +
> +int a, c = 1;
> +unsigned char b[1], d;
> +int main() {
> + while (a || !c) {
> + signed char e = b[1000000000];
> + d = e < 0 || b[1000000000] > 1;
> + if (d)
> + __builtin_abort ();
> + }
> + return 0;
> +}
> diff --git a/gcc/tree-eh.cc b/gcc/tree-eh.cc
> index 1033b124e4df3..08a106965b43f 100644
> --- a/gcc/tree-eh.cc
> +++ b/gcc/tree-eh.cc
> @@ -2646,6 +2646,27 @@ range_in_array_bounds_p (tree ref)
> return true;
> }
>
> +/* Return true iff a BIT_FIELD_REF <EXPR, SIZE, OFFSET> would access a bit
> + range that is known to be in bounds for EXPR's type. */
> +
> +bool
> +bit_field_ref_in_bounds_p (tree expr, poly_uint64 size, poly_uint64 offset)
> +{
> + tree expr_size_tree;
> + poly_uint64 expr_size_max, min = offset, wid = size, max;
> +
> + expr_size_tree = TYPE_SIZE (TREE_TYPE (expr));
> + if (!expr_size_tree || !poly_int_tree_p (expr_size_tree, &expr_size_max))
> + return false;
> +
> + max = min + wid;
> + if (maybe_lt (max, min)
> + || maybe_lt (expr_size_max, max))
> + return false;
> +
> + return true;
> +}
> +
> /* Return true if EXPR can trap, as in dereferencing an invalid pointer
> location or floating point arithmetic. C.f. the rtl version, may_trap_p.
> This routine expects only GIMPLE lhs or rhs input. */
> @@ -2688,10 +2709,17 @@ tree_could_trap_p (tree expr)
> restart:
> switch (code)
> {
> + case BIT_FIELD_REF:
> + if (DECL_P (TREE_OPERAND (expr, 0))
> + && !bit_field_ref_in_bounds_p (TREE_OPERAND (expr, 0),
> + bit_field_size (expr),
> + bit_field_offset (expr)))
> + return true;
> + /* Fall through. */
> +
> case COMPONENT_REF:
> case REALPART_EXPR:
> case IMAGPART_EXPR:
> - case BIT_FIELD_REF:
> case VIEW_CONVERT_EXPR:
> case WITH_SIZE_EXPR:
> expr = TREE_OPERAND (expr, 0);
> diff --git a/gcc/tree-eh.h b/gcc/tree-eh.h
> index 69fe193f1b82f..1c6f70b24fab7 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 bit_field_ref_in_bounds_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
