On Sat, Jan 18, 2025 at 7:02 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
>
> 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.
I think this is a bug in tree_could_trap_p which is indeed not considering
out-of-bound accesses of a VAR_DECL (or any decl), but very conservatively
handle ARRAY_REFs.
IMO the proper fix might be to use something like get_ref_base_and_extent
when analyzing the a tcc_reference sub-tree. Can you open a bugreport
to track this? I might want to take that.
For the issue at hand ...
> This may drop some ifcombine 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?
>
>
> 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.
>
> for gcc/testsuite/ChangeLog
>
> PR tree-optimization/118514
> * gcc.dg/field-merge-23.c: New.
> ---
> gcc/gimple-fold.cc | 19 +++++++++++++++----
> gcc/testsuite/gcc.dg/field-merge-23.c | 19 +++++++++++++++++++
> 2 files changed, 34 insertions(+), 4 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 3c971a29ef045..41b21ecd58a32 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -7686,10 +7686,16 @@ 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)))
> + && (!tree_fits_uhwi_p (TYPE_SIZE (TREE_TYPE (inner)))
> + || compare_tree_int (TYPE_SIZE (TREE_TYPE (inner)),
> + bp + bs) < 0))
So you fear of missed optimizations when allowing variable size types?
I'd have said we should have proof that the generated BIT_FIELD_REF
does not access out-of-bounds, in particular the
tree_could_trap_p (gimple_assign_rhs1 (load))
& !tree_could_trap_p (inner)
(&& btw) looks like bad style.
That said, the testcase in the PR is a bit artificial, so we might get away with
ignoring the issue for GCC 15 and go for fixing tree_could_trap_p?
Did you consider not applying this ifcombine to tree_could_trap_p original refs
at all? The short-circuiting that's removed should make this
susceptible anyhow?
Thanks,
Richard.
> || (INTEGRAL_TYPE_P (TREE_TYPE (inner))
> && !type_has_mode_precision_p (TREE_TYPE (inner))))
> return NULL_TREE;
> @@ -7859,6 +7865,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);
> 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;
> +}
>
> --
> 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
