On Mon, Nov 27, 2023 at 10:16 AM Martin Jambor <[email protected]> wrote:
>
> Hi,
>
> The enhancement to address PR 109849 contained an importsnt thinko,
> and that any reference that is passed to a function and does not
> escape, must also not happen to be aliased by the return value of the
> function. This has quickly transpired as bugs PR 112711 and PR
> 112721.
>
> Just as IPA-modref does a good enough job to allow us to rely on the
> escaped set of variables, it sems to be doing well also on updating
> EAF_NOT_RETURNED_DIRECTLY call argument flag which happens to address
> exactly the situation we need to avoid. Of course, if a call
> statement ignores any returned value, we also do not need to check the
> flag.
>
> Hopefully this does not pessimize things too much, I have verified
> that the PR 109849 testcae remains quick and so should also the
> benchmark it is derived from.
>
> The patch has passed bootstrap and testing on x86_64-linux, OK for
> master?
>
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2023-11-27 Martin Jambor <[email protected]>
>
> PR tree-optimization/112711
> PR tree-optimization/112721
> * tree-sra.cc (build_access_from_call_arg): New parameter
> CAN_BE_RETURNED, disqualify any candidate passed by reference if it is
> true. Adjust leading comment.
> (scan_function): Pass appropriate value to CAN_BE_RETURNED of
> build_access_from_call_arg.
>
> gcc/testsuite/ChangeLog:
>
> 2023-11-27 Martin Jambor <[email protected]>
>
> PR tree-optimization/112711
> PR tree-optimization/112721
> * g++.dg/tree-ssa/pr112711.C: New test.
> * gcc.dg/tree-ssa/pr112721.c: Likewise.
> ---
> gcc/testsuite/g++.dg/tree-ssa/pr112711.C | 31 ++++++++++++++++++
> gcc/testsuite/gcc.dg/tree-ssa/pr112721.c | 26 +++++++++++++++
> gcc/tree-sra.cc | 40 ++++++++++++++++++------
> 3 files changed, 88 insertions(+), 9 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr112711.C
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr112721.c
>
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr112711.C
> b/gcc/testsuite/g++.dg/tree-ssa/pr112711.C
> new file mode 100644
> index 00000000000..c04524b04a7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-ssa/pr112711.C
> @@ -0,0 +1,31 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1" } */
> +
> +typedef int i32;
> +typedef unsigned int u32;
> +
> +static inline void write_i32(void *memory, i32 value) {
> + // swap i32 bytes as if it was u32:
> + u32 u_value = value;
> + value = __builtin_bswap32(u_value);
> +
> + // llvm infers '1' alignment from destination type
> + __builtin_memcpy(__builtin_assume_aligned(memory, 1), &value,
> sizeof(value));
> +}
> +
> +__attribute__((noipa))
> +static void bug (void) {
> + #define assert_eq(lhs, rhs) if (lhs != rhs) __builtin_trap()
> +
> + unsigned char data[5];
> + write_i32(data, -1362446643);
> + assert_eq(data[0], 0xAE);
> + assert_eq(data[1], 0xCA);
> + write_i32(data + 1, -1362446643);
> + assert_eq(data[1], 0xAE);
> +}
Only a comment on this testcase, it is only valid for little-endian
and 32bit int targets.
You can modify it to fix it for both though.
Thanks,
Andrew
> +
> +int main() {
> + bug();
> + return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr112721.c
> b/gcc/testsuite/gcc.dg/tree-ssa/pr112721.c
> new file mode 100644
> index 00000000000..adf62613266
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr112721.c
> @@ -0,0 +1,26 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1" } */
> +
> +unsigned * volatile gv;
> +
> +struct a {
> + int b;
> +};
> +int c, e;
> +long d;
> +unsigned * __attribute__((noinline))
> +f(unsigned *g) {
> + for (; c;)
> + e = d;
> + return gv ? gv : g;
> +}
> +int main() {
> + int *h;
> + struct a i = {8};
> + int *j = &i.b;
> + h = (unsigned *) f(j);
> + *h = 0;
> + if (i.b != 0)
> + __builtin_abort ();
> + return 0;
> +}
> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> index 3a0d52675fe..6a759783990 100644
> --- a/gcc/tree-sra.cc
> +++ b/gcc/tree-sra.cc
> @@ -1268,18 +1268,27 @@ abnormal_edge_after_stmt_p (gimple *stmt, enum
> out_edge_check *oe_check)
> }
>
> /* Scan expression EXPR which is an argument of a call and create access
> - structures for all accesses to candidates for scalarization. Return true
> if
> - any access has been inserted. STMT must be the statement from which the
> - expression is taken. */
> + structures for all accesses to candidates for scalarization. Return true
> + if any access has been inserted. STMT must be the statement from which
> the
> + expression is taken. CAN_BE_RETURNED must be true if call argument flags
> + do not rule out that the argument is directly returned. OE_CHECK is used
> + to remember result of a test for abnormal outgoing edges after this
> + statement. */
>
> static bool
> -build_access_from_call_arg (tree expr, gimple *stmt,
> +build_access_from_call_arg (tree expr, gimple *stmt, bool can_be_returned,
> enum out_edge_check *oe_check)
> {
> if (TREE_CODE (expr) == ADDR_EXPR)
> {
> tree base = get_base_address (TREE_OPERAND (expr, 0));
>
> + if (can_be_returned)
> + {
> + disqualify_base_of_expr (base, "Address possibly returned, "
> + "leading to an alis SRA may not know.");
> + return false;
> + }
> if (abnormal_edge_after_stmt_p (stmt, oe_check))
> {
> disqualify_base_of_expr (base, "May lead to need to add statements "
> @@ -1508,12 +1517,25 @@ scan_function (void)
> case GIMPLE_CALL:
> {
> enum out_edge_check oe_check = SRA_OUTGOING_EDGES_UNCHECKED;
> - for (i = 0; i < gimple_call_num_args (stmt); i++)
> - ret |= build_access_from_call_arg (gimple_call_arg (stmt,
> i),
> - stmt, &oe_check);
> + gcall *call = as_a <gcall *> (stmt);
> + for (i = 0; i < gimple_call_num_args (call); i++)
> + {
> + bool can_be_returned;
> + if (gimple_call_lhs (call))
> + {
> + int af = gimple_call_arg_flags (call, i);
> + can_be_returned = !(af & EAF_NOT_RETURNED_DIRECTLY);
> + }
> + else
> + can_be_returned = false;
> + ret |= build_access_from_call_arg (gimple_call_arg (call,
> + i),
> + stmt, can_be_returned,
> + &oe_check);
> + }
> if (gimple_call_chain(stmt))
> - ret |= build_access_from_call_arg (gimple_call_chain(stmt),
> - stmt, &oe_check);
> + ret |= build_access_from_call_arg (gimple_call_chain(call),
> + stmt, false, &oe_check);
> }
>
> t = gimple_call_lhs (stmt);
> --
> 2.43.0
>
