Patchwork fix cross build

login
register
mail settings
Submitter Richard Guenther
Date May 22, 2012, 2:12 p.m.
Message ID <CAFiYyc3HpkaR+j-PiH+RNJ9Ava86Vf2eyu0GZ5VKrRK0Kz1Thg@mail.gmail.com>
Download mbox | patch
Permalink /patch/160646/
State New
Headers show

Comments

Richard Guenther - May 22, 2012, 2:12 p.m.
On Tue, May 22, 2012 at 3:24 PM, Nathan Sidwell <nathan@acm.org> wrote:
> On 05/21/12 11:03, Richard Guenther wrote:
>
>> Hmm - I think this papers over the issue that the CONSTRUCTOR is not
>> properly gimplified - it still contains a TARGET_EXPR which is not valid
>> GIMPLE.
>> Why is that TARGET_EXPR not gimplified?
>
>
> As far as I can make out, it just doesn't look inside the constructor.
>
> 0  gimplify_expr (expr_p=0xb7e90394, pre_p=0xbfffe514, post_p=0xbfffd08c,
>    gimple_test_f=0x84b0a80 <is_gimple_min_lval(tree_node*)>, fallback=3) at
> ../../src/gcc/gimplify.c:7356
> #1  0x084f5882 in gimplify_compound_lval (expr_p=0xb7e9cb94,
> pre_p=0xbfffe514, post_p=0xbfffd08c, fallback=1)
>    at ../../src/gcc/gimplify.c:2259
> #2  0x08519878 in gimplify_expr (expr_p=0xb7e9cb94, pre_p=0xbfffe514,
> post_p=0xbfffd08c,
>    gimple_test_f=0x84eaf9f <is_gimple_reg_rhs_or_call(tree)>, fallback=1) at
> ../../src/gcc/gimplify.c:7081
> #3  0x085087f7 in gimplify_modify_expr (expr_p=0xbfffd6d0, pre_p=0xbfffe514,
> post_p=0xbfffd08c, want_value=false)
>    at ../../src/gcc/gimplify.c:4843
>
> The switch case at that stack frame is for CONSTRUCTORs.  It has the
> comment:
> /* Don't reduce this in place; let gimplify_init_constructor work its
>             magic.  Buf if we're just elaborating this for side effects,
> just
>             gimplify any element that has side-effects.  */
>
> But we never enter G_I_C before the ICE.
>
> On the second time we get here, fallback has the value 1 at that point
> (fb_rvalue), so neither if condition is satisified, and we end up at
>            ret = GS_ALL_DONE;
> we then proceed down to enter:
>  else if ((fallback & fb_rvalue) && is_gimple_reg_rhs_or_call (*expr_p))
>    {
>      /* An rvalue will do.  Assign the gimplified expression into a
>         new temporary TMP and replace the original expression with
>         TMP.  First, make sure that the expression has a type so that
>         it can be assigned into a temporary.  */
>      gcc_assert (!VOID_TYPE_P (TREE_TYPE (*expr_p)));
>      *expr_p = get_formal_tmp_var (*expr_p, pre_p);
>    }
>
> and ICE inside GFTV when it attempts to generate the hash.
>
> AFAICT, changing the test case to:
>  char *str = ((union {const char * _q; char * _nq;})
>               ((const char *)((num_string)
>                               ->string.str)))._nq;
> (i.e. removing the stmt-expr) results in the same logical flow as above, but
> there's no TARGET_EXPR inside the constructor.
>
> I'm not sure how it's supposed to work, so I'm a little lost.

Hm, ok.  I see what happens.  The issue is that the CONSTRUCTOR has
elements with TREE_SIDE_EFFECTS set but is itself not TREE_SIDE_EFFECTS.
Which would have avoided all this mess in lookup_tmp_var.  I suppose
for duplicating the initializer you could even generate wrong-code with your fix
in(?).  Now, we never set TREE_SIDE_EFFECTS from build_constructor
(but only TREE_CONSTANT), so the fix could either be to fix that or to
exclude CONSTRUCTORs in lookup_tmp_var like


after all lookup_tmp_var performs a simple-minded CSE here.

But I wonder why CONSTRUCTORs do not inherit TREE_SIDE_EFFECTS
properly ...

Richard.
Nathan Sidwell - May 22, 2012, 3:18 p.m.
On 05/22/12 15:12, Richard Guenther wrote:

thanks!

> But I wonder why CONSTRUCTORs do not inherit TREE_SIDE_EFFECTS
> properly ...

yeah, that would seem to be the error.  Looking ...

Patch

Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c      (revision 187772)
+++ gcc/gimplify.c      (working copy)
@@ -514,7 +514,8 @@  lookup_tmp_var (tree val, bool is_formal
      block, which means it will go into memory, causing much extra
      work in reload and final and poorer code generation, outweighing
      the extra memory allocation here.  */
-  if (!optimize || !is_formal || TREE_SIDE_EFFECTS (val))
+  if (!optimize || !is_formal || TREE_SIDE_EFFECTS (val)
+      || TREE_CODE (val) == CONSTRUCTOR)
     ret = create_tmp_from_val (val);
   else
     {