Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

["Richard Guenther" <richard dot guenther at gmail dot com>]

On 10/2/07, Richard Sandiford <rsandifo@nildram.co.uk> wrote:
> The following code:
>
> ---------------------------------------------------------------------------
> typedef float V8SF __attribute__ ((vector_size (32)));
> void test0 (V8SF);
> void
> foo (float x)
> {
>   test ((V8SF) { x, x, x, x, x, x, x, x });
> }
> ---------------------------------------------------------------------------
>
> causes an ICE on targets for which the vector argument is callee-copied:
>
> ---------------------------------------------------------------------------
> /tmp/foo.c: In function 'foo':
> /tmp/foo.c:6: internal compiler error: in copy_constant, at varasm.c:3067
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
> ---------------------------------------------------------------------------
>
> We try to take the address of the original CONSTRUCTOR (rather than
> taking the address of a temporary as in the caller-copied case) and
> expand_expr_addr_expr_1 assumes that all CONSTRUCTORs are constant:
>
>   /* If we are taking the address of a constant and are at the top level,
>      we have to use output_constant_def since we can't call force_const_mem
>      at top level.  */
>   /* ??? This should be considered a front-end bug.  We should not be
>      generating ADDR_EXPR of something that isn't an LVALUE.  The only
>      exception here is STRING_CST.  */
>   if (TREE_CODE (exp) == CONSTRUCTOR
>       || CONSTANT_CLASS_P (exp))
>     return XEXP (expand_expr_constant (exp, 0, modifier), 0);
>
> However, the comment doesn't really seem to fit this case:
> the CONSTRUCTOR _is_ an lvalue.  In the post-gimple world,
> all real constant CONSTRUCTORs (i.e. those that can be put
> in a readonly data segment) will already have been turned
> into decls, even for -O0.  And it seems perfectly valid to
> take the address of the non-constant constructor above.
>
> The patch therefore treats CONSTRUCTORs in the same way as
> decls and language-specific codes, namely by passing them
> through expand_expr.  This will yield a MEM for the
> constructor contents.
>
> (If the agreement is that it isn't in fact valid to take the address of
> a constructor, we could instead check TREE_CODE (base) != CONSTRUCTOR in
> the following calls.c code:
>
>           /* If we're compiling a thunk, pass through invisible references
>              instead of making a copy.  */
>           if (call_from_thunk_p
>               || (callee_copies
>                   && !TREE_ADDRESSABLE (type)
>                   && (base = get_base_address (args[i].tree_value))
>                   && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
>
> We'd then treat callee-copied constructors exactly like caller-copied ones.)
>
> This patch fixes gcc.c-torture/compile/20050113-1.c for mips32-elf.
> However, that test is really for something else, and because its
> vector is only 8 bytes wide, it doesn't fail for mipsisa64-elf.
> I've therefore added a new test specifically for this bug.
> This version fails on both targets.
>
> Bootstrapped & regression-tested on x86_64-linux-gnu.  Also
> regression-tested on mipsisa32-elf.  OK to install?

Ok.  I wonder if ...

> +        constructor's contents.  Assume language specific tree nodes can
> +        be expanded in some interesting way.  */
>        if (DECL_P (exp)
> +         || TREE_CODE (exp) == CONSTRUCTOR
>           || TREE_CODE (exp) >= LAST_AND_UNUSED_TREE_CODE)
>         {

... we ever end up with lang-specific tree nodes here.  If you feel
eager to try,
a patch to remove that case is pre-approved if it passes bootstrap & regtest.

Thanks,
Richard.