Patchwork force_const_mem VOIDmode

login
register
mail settings
Submitter Alan Modra
Date June 7, 2013, 2 a.m.
Message ID <20130607020053.GJ6878@bubble.grove.modra.org>
Download mbox | patch
Permalink /patch/249578/
State New
Headers show

Comments

Alan Modra - June 7, 2013, 2 a.m.
force_const_mem() isn't supposed to handle VOIDmode or BLKmode, so the
check for VOIDmode when aligning is needless.  If we ever did get one
of these modes in a constant pool, this

  pool->offset += GET_MODE_SIZE (mode);

won't add to the pool size, and output_constant_pool_2() will hit a
gcc_unreachable().  Bootstrapped etc. powerpc64-linux.

	* varasm.c (force_const_mem): Assert mode is not VOID or BLK.
Richard Guenther - June 7, 2013, 8:47 a.m.
On Fri, Jun 7, 2013 at 4:00 AM, Alan Modra <amodra@gmail.com> wrote:
> force_const_mem() isn't supposed to handle VOIDmode or BLKmode, so the
> check for VOIDmode when aligning is needless.  If we ever did get one
> of these modes in a constant pool, this
>
>   pool->offset += GET_MODE_SIZE (mode);
>
> won't add to the pool size, and output_constant_pool_2() will hit a
> gcc_unreachable().  Bootstrapped etc. powerpc64-linux.

Ok.

Thanks,
Richard.

>         * varasm.c (force_const_mem): Assert mode is not VOID or BLK.
>
> Index: gcc/varasm.c
> ===================================================================
> --- gcc/varasm.c        (revision 199718)
> +++ gcc/varasm.c        (working copy)
> @@ -3567,7 +3575,8 @@
>    *slot = desc;
>
>    /* Align the location counter as required by EXP's data type.  */
> -  align = GET_MODE_ALIGNMENT (mode == VOIDmode ? word_mode : mode);
> +  gcc_checking_assert (mode != VOIDmode && mode != BLKmode);
> +  align = GET_MODE_ALIGNMENT (mode);
>  #ifdef CONSTANT_ALIGNMENT
>    {
>      tree type = lang_hooks.types.type_for_mode (mode, 0);
>
> --
> Alan Modra
> Australia Development Lab, IBM
Andreas Krebbel - June 13, 2013, 2:29 p.m.
On 07/06/13 04:00, Alan Modra wrote:
> force_const_mem() isn't supposed to handle VOIDmode or BLKmode, so the
> check for VOIDmode when aligning is needless.  If we ever did get one
> of these modes in a constant pool, this
> 
>   pool->offset += GET_MODE_SIZE (mode);

Hi Alan,

the assertion is triggered on S/390 during bootstrap.

The S/390 movmem_short expander emits the following insn:

(insn 793 3147 795 93 (parallel [
            (set (mem:BLK (reg/f:SI 509 [ gi_filename ]) [0 A8])
                (mem:BLK (reg/f:SI 510 [ gcov_prefix ]) [0 A8]))
            (use (reg:SI 511 [ prefix_length ]))
            (use (const:BLK (unspec:BLK [
                            (const_int 0 [0])
                        ] UNSPEC_INSN)))
            (clobber (reg:SI 1288))
        ])

For machines without larl (load address relative long) the RTX:

(const:BLK (unspec:BLK [
                            (const_int 0 [0])
                        ] UNSPEC_INSN))

is supposed to be pushed into the literal pool. The expression represents the target of the execute
instruction. It doesn't matter that the literal pool offset calculation doesn't work with this RTX
the backend does this in machine dependent reorg anyway on its own.

Picking a different mode wouldn't be correct for our purposes since the size depends on the pattern
containing the expression.

So from a S/390 perspective I think the !BLKmode assertion should be removed.

Bye,

-Andreas-


> 
> won't add to the pool size, and output_constant_pool_2() will hit a
> gcc_unreachable().  Bootstrapped etc. powerpc64-linux.
> 
> 	* varasm.c (force_const_mem): Assert mode is not VOID or BLK.
> 
> Index: gcc/varasm.c
> ===================================================================
> --- gcc/varasm.c	(revision 199718)
> +++ gcc/varasm.c	(working copy)
> @@ -3567,7 +3575,8 @@
>    *slot = desc;
> 
>    /* Align the location counter as required by EXP's data type.  */
> -  align = GET_MODE_ALIGNMENT (mode == VOIDmode ? word_mode : mode);
> +  gcc_checking_assert (mode != VOIDmode && mode != BLKmode);
> +  align = GET_MODE_ALIGNMENT (mode);
>  #ifdef CONSTANT_ALIGNMENT
>    {
>      tree type = lang_hooks.types.type_for_mode (mode, 0);
>
Alan Modra - June 13, 2013, 3:31 p.m.
On Thu, Jun 13, 2013 at 04:29:44PM +0200, Andreas Krebbel wrote:
> On 07/06/13 04:00, Alan Modra wrote:
> > force_const_mem() isn't supposed to handle VOIDmode or BLKmode, so the
> > check for VOIDmode when aligning is needless.  If we ever did get one
> > of these modes in a constant pool, this
> > 
> >   pool->offset += GET_MODE_SIZE (mode);
> 
> Hi Alan,
> 
> the assertion is triggered on S/390 during bootstrap.

Reverting the patch seems the simplest thing to do, so I've done that.
Apologies for the breakage.

Patch

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 199718)
+++ gcc/varasm.c	(working copy)
@@ -3567,7 +3575,8 @@ 
   *slot = desc;
 
   /* Align the location counter as required by EXP's data type.  */
-  align = GET_MODE_ALIGNMENT (mode == VOIDmode ? word_mode : mode);
+  gcc_checking_assert (mode != VOIDmode && mode != BLKmode);
+  align = GET_MODE_ALIGNMENT (mode);
 #ifdef CONSTANT_ALIGNMENT
   {
     tree type = lang_hooks.types.type_for_mode (mode, 0);