| Message ID | 20101215175901.GJ27214@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/15/2010 9:59 AM, Jakub Jelinek wrote: > CCing Mark who has added alt_rtl stuff. Unfortunately, I remember nothing about this. I went and looked at the code, and I just don't fully recall what was going on there. > I think either we just shouldn't set alt_rtl at all when it has side effects > (as done in this patch), or the above if should do similar testing on > side_effects_p for alt_rtl/target. I suspect the latter. I think that it's more logical for the low-level function (expand_expr_real_1) to say "this is what's being modified" and for the high-level one to say "OK, but I don't care" than it is for the low-level function to try to guess why the high-level one wants the information. I suppose that's just style, though, not substance. Thank you,
--- gcc/expr.c.jj 2010-11-29 13:27:43.000000000 +0100 +++ gcc/expr.c 2010-12-14 14:12:07.000000000 +0100 @@ -8445,7 +8445,7 @@ expand_expr_real_1 (tree exp, rtx target else if (MEM_P (decl_rtl) && modifier != EXPAND_INITIALIZER) { - if (alt_rtl) + if (alt_rtl && !side_effects_p (decl_rtl)) *alt_rtl = decl_rtl; decl_rtl = use_anchored_address (decl_rtl); if (modifier != EXPAND_CONST_ADDRESS --- gcc/testsuite/gcc.target/i386/pr45852.c.jj 2010-12-14 14:30:23.000000000 +0100 +++ gcc/testsuite/gcc.target/i386/pr45852.c 2010-12-14 14:37:59.000000000 +0100 @@ -0,0 +1,16 @@ +/* PR middle-end/45852 */ +/* { dg-options "-O2 -mcmodel=small" } */ +/* { dg-do compile { target { { i?86-*-linux* x86_64-*-linux* } && lp64 } } } */ +/* { dg-require-visibility "" } */ + +struct S { int s; }; + +volatile struct S globvar __attribute__((visibility ("hidden"))) = { -6 }; + +void +foo (void) +{ + globvar = globvar; +} + +/* { dg-final { scan-assembler-times "globvar.%?rip" 2 } } */
Hi! For volatile aggregate assigned to self expand optimizes the load and store completely. The problem is that while in store_expr we check for side-effects when checking temp vs. target: if ((! rtx_equal_p (temp, target) || (temp != target && (side_effects_p (temp) || side_effects_p (target)))) && TREE_CODE (exp) != ERROR_MARK /* If store_expr stores a DECL whose DECL_RTL(exp) == TARGET, but TARGET is not valid memory reference, TEMP will differ from TARGET although it is really the same location. */ && !(alt_rtl && rtx_equal_p (alt_rtl, target)) /* If there's nothing to copy, don't bother. Don't call expr_size unless necessary, because some front-ends (C++) expr_size-hook must not be given objects that are not supposed to be bit-copied or bit-initialized. */ && expr_size (exp) != const0_rtx) (as MEMs are copy_rtx'ed, above temp != target is true, they are rtx_equal_p but both have side_effects_p), when checking alt_rtl (which in this case is also rtx_equal_p to both target and temp) no side_effects_p checks are done and so the above if is false (which means no code is emitted). CCing Mark who has added alt_rtl stuff. I think either we just shouldn't set alt_rtl at all when it has side effects (as done in this patch), or the above if should do similar testing on side_effects_p for alt_rtl/target. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2010-12-14 Jakub Jelinek <jakub@redhat.com> PR middle-end/45852 * expr.c (expand_expr_real_1): Don't set *alt_rtl if decl_rtl is a volatile MEM. * gcc.target/i386/pr45852.c: New test. Jakub