Here's two approaches to fix the regression in 89689. Note this only
fixes the regression in the originally reported testcase. THe deeper
issue Martin raises in C#1 is not addressed. Thus if we go forward
with either patch ISTM that we would drop the regression markers, but
keep the BZ open.
So the key blocks in question as we enter DSE1:
>
> ;; basic block 2, loop depth 0, maybe hot
> ;; prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
> ;; pred: ENTRY (FALLTHRU,EXECUTABLE)
> h = l;
> h$data_33 = l.data;
> if (h$data_33 != &__sb_slop)
> goto <bb 3>; [70.00%]
> else
> goto <bb 4>; [30.00%]
> ;; succ: 3 [70.0% (guessed)] (TRUE_VALUE,EXECUTABLE)
> ;; 4 [30.0% (guessed)] (FALSE_VALUE,EXECUTABLE)
>
> ;; basic block 3, loop depth 0, maybe hot
> ;; prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
> ;; pred: 2 [70.0% (guessed)] (TRUE_VALUE,EXECUTABLE)
> *h$data_33 = 0;
> ;; succ: 4 [always] (FALLTHRU,EXECUTABLE)
>
> ;; basic block 4, loop depth 0, maybe hot
> ;; prev block 3, next block 5, flags: (NEW, REACHABLE, VISITED)
> ;; pred: 3 [always] (FALLTHRU,EXECUTABLE)
> ;; 2 [30.0% (guessed)] (FALSE_VALUE,EXECUTABLE)
> _23 = __builtin_object_size (h$data_33, 0);
> _25 = __builtin___memcpy_chk (h$data_33, "abcd", 4, _23);
> __builtin_puts (h$data_33);
> _6 = h$data_33 == &__sb_slop;
> _7 = (int) _6;
> printf ("%d\n", _7);
> _9 = h$data_33 == &buf;
> _10 = (int) _9;
> printf ("%d\n", _10);
> if (h$data_33 != &__sb_slop)
> goto <bb 5>; [70.00%]
> else
> goto <bb 6>; [30.00%]
>
So vrp1 is going to want to thread the jump at the end of bb4 which
requires duplicating bb4. One version of bb4 will only be reachable
via the false edge from bb2. That in turn exposes a cprop opportunity
to replace h$data_33 with &__sbloop and the type of &__sbloop is a
char[1] array thus triggering the false positive warning.
We can avoid all that by realizing the store in bb3 is dead. That in
turn makes the conditional at the end of bb2 pointless as bb3 is empty
and thus both arms ultimately transfer control to bb4 without any other
side effects meaning we can eliminate that conditional which in turn
eliminates the need for jump threading.
Again, this only fixes the original testcase and if there were other
statements in bb3 it wouldn't work and it won't work for the test in
c#1. But the proposed changes are a general improvement.
DSE misses this case because ref_maybe_used_by_stmt_p returns true for
the virtual operand of the __builtin_object_size call. Opps!
There's two easy ways to fix this, I've bootstrapped and regression
tested both.
First, the most conservative fix and perhaps the most appropriate for
gcc-10 -- special casing __builtin_object_size in tree-ssa-dse.c.
The second approach is more general and marks __builtin_object_size as
const rather than just pure. Jakub and I kicked this around in IRC a
bit. We've always marked __builtin_object_size as pure. It was never
const. THere is some concern that code motion might cause _b_o_s to
get different results, which would be particularly concerning for
_b_o_s types #1 and #3 which do use some contextual information.
However, ISTM that motion is still going to be limited by the SSA
graph, though somewhat less because we wouldn't have a dependency on
the virtual operands. So it *should* be safe, but there's some degree
of hesitation to make this change at this point in gcc-10's lifecycle.
Thoughts? If we go forward obviously I'd add a testcase and ChangeLog
entries.
Jeff
diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
index 374143e5785..e88342f1b68 100644
--- a/gcc/tree-ssa-dse.c
+++ b/gcc/tree-ssa-dse.c
@@ -789,8 +789,13 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
phi_def = use_stmt;
}
}
- /* If the statement is a use the store is not dead. */
- else if (ref_maybe_used_by_stmt_p (use_stmt, ref))
+ /* If the statement is a use the store is not dead. Do not
+ consider calls to __builtin_object_size as a use. For gcc-11
+ we plan mark __builtin_object_size as const. */
+ else if ((!is_gimple_call (use_stmt)
+ || !gimple_call_builtin_p (as_a <gcall *> (use_stmt),
+ BUILT_IN_OBJECT_SIZE))
+ && ref_maybe_used_by_stmt_p (use_stmt, ref))
{
/* Handle common cases where we can easily build an ao_ref
structure for USE_STMT and in doing so we find that the
commit f4e5d3f4755b6a5846ac20b53008b90131a8bb7c
Author: Jeff Law <l...@torsion.usersys.redhat.com>
Date: Tue Jan 28 18:16:09 2020 -0500
Mark _b_o_s as constant
diff --git a/gcc/builtins.def b/gcc/builtins.def
index 5ab842c34c2..fa8b0641ab1 100644
--- a/gcc/builtins.def
+++ b/gcc/builtins.def
@@ -972,7 +972,7 @@ DEF_BUILTIN_STUB (BUILT_IN_STRCMP_EQ, "__builtin_strcmp_eq")
DEF_BUILTIN_STUB (BUILT_IN_STRNCMP_EQ, "__builtin_strncmp_eq")
/* Object size checking builtins. */
-DEF_GCC_BUILTIN (BUILT_IN_OBJECT_SIZE, "object_size",
BT_FN_SIZE_CONST_PTR_INT, ATTR_PURE_NOTHROW_LEAF_LIST)
+DEF_GCC_BUILTIN (BUILT_IN_OBJECT_SIZE, "object_size",
BT_FN_SIZE_CONST_PTR_INT, ATTR_CONST_NOTHROW_LEAF_LIST)
DEF_EXT_LIB_BUILTIN (BUILT_IN_MEMCPY_CHK, "__memcpy_chk",
BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
DEF_EXT_LIB_BUILTIN (BUILT_IN_MEMMOVE_CHK, "__memmove_chk",
BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
DEF_EXT_LIB_BUILTIN (BUILT_IN_MEMPCPY_CHK, "__mempcpy_chk",
BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF)
