| Message ID | Zg0IB+apdMZcKCy9@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | expr: Fix up emit_push_insn [PR114552] | expand |
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 > >
--- 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; +}