On Wed, 3 Apr 2024, Jakub Jelinek wrote:
> Hi!
>
> r13-990 added optimizations in multiple spots to optimize during
> expansion storing of constant initializers into targets.
> In the load_register_parameters and expand_expr_real_1 cases,
> it checks it has a tree as the source and so knows we are reading
> that whole decl's value, so the code is fine as is, but in the
> emit_push_insn case it checks for a MEM from which something
> is pushed and checks for SYMBOL_REF as the MEM's address, but
> still assumes the whole object is copied, which as the following
> testcase shows might not always be the case. In the testcase,
> k is 6 bytes, then 2 bytes of padding, then another 4 bytes,
> while the emit_push_insn wants to store just the 6 bytes.
>
> The following patch simply verifies it is the whole initializer
> that is being stored, I think that is best thing to do so late
> in GCC 14 cycle as well for backporting.
>
> For GCC 15, perhaps the code could stop requiring it must be at offset zero,
> nor that the size is a subset, but could use
> get_symbol_constant_value/fold_ctor_reference gimple-fold APIs to actually
> extract just part of the initializer if we e.g. push just some subset
> (of course, still verify that it is a subset). For sizes which are power
> of two bytes and we have some integer modes, we could use as type for
> fold_ctor_reference corresponding integral types, otherwise dunno, punt
> or use some structure (e.g. try to find one in the initializer?), whatever.
> But even in the other spots it could perhaps handle loading of
> COMPONENT_REFs or MEM_REFs from the .rodata vars.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Thanks,
Richard.
> 2024-04-02 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/114552
> * expr.cc (emit_push_insn): Only use store_constructor for
> immediate_const_ctor_p if int_expr_size matches size.
>
> * gcc.c-torture/execute/pr114552.c: New test.
>
> --- gcc/expr.cc.jj 2024-03-15 10:10:51.209237835 +0100
> +++ gcc/expr.cc 2024-04-02 16:01:39.566744302 +0200
> @@ -5466,6 +5466,7 @@ emit_push_insn (rtx x, machine_mode mode
> /* If source is a constant VAR_DECL with a simple constructor,
> store the constructor to the stack instead of moving it. */
> const_tree decl;
> + HOST_WIDE_INT sz;
> if (partial == 0
> && MEM_P (xinner)
> && SYMBOL_REF_P (XEXP (xinner, 0))
> @@ -5473,9 +5474,11 @@ emit_push_insn (rtx x, machine_mode mode
> && VAR_P (decl)
> && TREE_READONLY (decl)
> && !TREE_SIDE_EFFECTS (decl)
> - && immediate_const_ctor_p (DECL_INITIAL (decl), 2))
> - store_constructor (DECL_INITIAL (decl), target, 0,
> - int_expr_size (DECL_INITIAL (decl)), false);
> + && immediate_const_ctor_p (DECL_INITIAL (decl), 2)
> + && (sz = int_expr_size (DECL_INITIAL (decl))) > 0
> + && CONST_INT_P (size)
> + && INTVAL (size) == sz)
> + store_constructor (DECL_INITIAL (decl), target, 0, sz, false);
> else
> emit_block_move (target, xinner, size, BLOCK_OP_CALL_PARM);
> }
> --- gcc/testsuite/gcc.c-torture/execute/pr114552.c.jj 2024-04-02
> 16:08:12.959366793 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr114552.c 2024-04-02
> 16:03:49.829963659 +0200
> @@ -0,0 +1,24 @@
> +/* PR middle-end/114552 */
> +
> +struct __attribute__((packed)) S { short b; int c; };
> +struct T { struct S b; int e; };
> +static const struct T k = { { 1, 0 }, 0 };
> +
> +__attribute__((noinline)) void
> +foo (void)
> +{
> + asm volatile ("" : : : "memory");
> +}
> +
> +__attribute__((noinline)) void
> +bar (struct S n)
> +{
> + foo ();
> +}
> +
> +int
> +main ()
> +{
> + bar (k.b);
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
