Re: [PATCH] expr: Fix up emit_push_insn [PR114552]

[Richard Biener <rguenther@suse.de>]

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  <jakub@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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)