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.
> 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)
|| (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))
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)))
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
