On Tue, May 22, 2012 at 3:24 PM, Nathan Sidwell <nat...@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
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
{
after all lookup_tmp_var performs a simple-minded CSE here.
But I wonder why CONSTRUCTORs do not inherit TREE_SIDE_EFFECTS
properly ...
Richard.
