[PATCH] Fix PR50494

[PATCH] Fix PR50494

[Richard Biener]

This should fix PR50494 - when we gimplify a local initializer
via a block copy and emit a constant to the constant pool we
stream the representative VAR_DECL with LTO, including its
eventually changed alignment.  When we then lookup RTL for this
constant at

  /* If this variable belongs to the global constant pool, retrieve the
     pre-computed RTL or recompute it in LTO mode.  */
  if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
    {
      SET_DECL_RTL (decl, output_constant_def (DECL_INITIAL (decl), 1));
      return;

we create yet another VAR_DECL for the constant representation.
But we ignore the alignment of the originally used representative
and instead use default alignment.  This breaks on powerpc
where the vectorizer increases alignment of such a representative
VAR_DECL coming from LTO (and thus not yet registered with the
constant output machinery), as the generated constant will not
have that increased alignment.

The fix should be to never create a new VAR_DECL for the
constant representation in the above path but re-use 'decl'
which has whatever alignment it has.

LTO bootstrap and regtest on x86_64-unknown-linux-gnu running
(not sure if this will excercise this a lot), I'm waiting for
ppc folks to confirm the bug is gone with the patch.

Eric, does this look correct?

Thanks,
Richard.

2013-02-13  Richard Biener  <rguenther@suse.de>

	PR lto/50494
	* varasm.c (output_constant_def_1): Get the decl representing
	the constant as argument.
	(output_constant_def): Wrap output_constant_def_1.
	(make_decl_rtl): Use output_constant_def_1 with the decl
	representing the constant.
	(build_constant_desc): Optionally re-use a decl already
	representing the constant.
	(tree_output_constant_def): Adjust.

Index: gcc/varasm.c
===================================================================
*** gcc/varasm.c	(revision 195997)
--- gcc/varasm.c	(working copy)
*************** static void asm_output_aligned_bss (FILE
*** 126,131 ****
--- 126,132 ----
  #endif /* BSS_SECTION_ASM_OP */
  static void mark_weak (tree);
  static void output_constant_pool (const char *, tree);
+ static rtx output_constant_def_1 (tree, tree, int);
  \f
  /* Well-known sections, each one associated with some sort of *_ASM_OP.  */
  section *text_section;
*************** make_decl_rtl (tree decl)
*** 1186,1192 ****
       pre-computed RTL or recompute it in LTO mode.  */
    if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
      {
!       SET_DECL_RTL (decl, output_constant_def (DECL_INITIAL (decl), 1));
        return;
      }
  
--- 1187,1194 ----
       pre-computed RTL or recompute it in LTO mode.  */
    if (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
      {
!       SET_DECL_RTL (decl, output_constant_def_1 (DECL_INITIAL (decl),
! 						 decl, 1));
        return;
      }
  
*************** get_constant_size (tree exp)
*** 3073,3088 ****
     Make a constant descriptor to enter EXP in the hash table.
     Assign the label number and construct RTL to refer to the
     constant's location in memory.
     Caller is responsible for updating the hash table.  */
  
  static struct constant_descriptor_tree *
! build_constant_desc (tree exp)
  {
    struct constant_descriptor_tree *desc;
    rtx symbol, rtl;
    char label[256];
    int labelno;
-   tree decl;
  
    desc = ggc_alloc_constant_descriptor_tree ();
    desc->value = copy_constant (exp);
--- 3075,3090 ----
     Make a constant descriptor to enter EXP in the hash table.
     Assign the label number and construct RTL to refer to the
     constant's location in memory.
+    If DECL is non-NULL use it as VAR_DECL associated with the constant.
     Caller is responsible for updating the hash table.  */
  
  static struct constant_descriptor_tree *
! build_constant_desc (tree exp, tree decl)
  {
    struct constant_descriptor_tree *desc;
    rtx symbol, rtl;
    char label[256];
    int labelno;
  
    desc = ggc_alloc_constant_descriptor_tree ();
    desc->value = copy_constant (exp);
*************** build_constant_desc (tree exp)
*** 3096,3123 ****
    ASM_GENERATE_INTERNAL_LABEL (label, "LC", labelno);
  
    /* Construct the VAR_DECL associated with the constant.  */
!   decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (label),
! 		     TREE_TYPE (exp));
!   DECL_ARTIFICIAL (decl) = 1;
!   DECL_IGNORED_P (decl) = 1;
!   TREE_READONLY (decl) = 1;
!   TREE_STATIC (decl) = 1;
!   TREE_ADDRESSABLE (decl) = 1;
!   /* We don't set the RTL yet as this would cause varpool to assume that the
!      variable is referenced.  Moreover, it would just be dropped in LTO mode.
!      Instead we set the flag that will be recognized in make_decl_rtl.  */
!   DECL_IN_CONSTANT_POOL (decl) = 1;
!   DECL_INITIAL (decl) = desc->value;
!   /* ??? CONSTANT_ALIGNMENT hasn't been updated for vector types on most
!      architectures so use DATA_ALIGNMENT as well, except for strings.  */
!   if (TREE_CODE (exp) == STRING_CST)
      {
  #ifdef CONSTANT_ALIGNMENT
!       DECL_ALIGN (decl) = CONSTANT_ALIGNMENT (exp, DECL_ALIGN (decl));
  #endif
      }
-   else
-     align_variable (decl, 0);
  
    /* Now construct the SYMBOL_REF and the MEM.  */
    if (use_object_blocks_p ())
--- 3098,3129 ----
    ASM_GENERATE_INTERNAL_LABEL (label, "LC", labelno);
  
    /* Construct the VAR_DECL associated with the constant.  */
!   if (decl == NULL_TREE)
      {
+       decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (label),
+ 			 TREE_TYPE (exp));
+       DECL_ARTIFICIAL (decl) = 1;
+       DECL_IGNORED_P (decl) = 1;
+       TREE_READONLY (decl) = 1;
+       TREE_STATIC (decl) = 1;
+       TREE_ADDRESSABLE (decl) = 1;
+       /* We don't set the RTL yet as this would cause varpool to assume that
+ 	 the variable is referenced.  Moreover, it would just be dropped in
+ 	 LTO mode.  Instead we set the flag that will be recognized in
+ 	 make_decl_rtl.  */
+       DECL_IN_CONSTANT_POOL (decl) = 1;
+       DECL_INITIAL (decl) = desc->value;
+       /* ??? CONSTANT_ALIGNMENT hasn't been updated for vector types on most
+ 	 architectures so use DATA_ALIGNMENT as well, except for strings.  */
+       if (TREE_CODE (exp) == STRING_CST)
+ 	{
  #ifdef CONSTANT_ALIGNMENT
! 	  DECL_ALIGN (decl) = CONSTANT_ALIGNMENT (exp, DECL_ALIGN (decl));
  #endif
+ 	}
+       else
+ 	align_variable (decl, 0);
      }
  
    /* Now construct the SYMBOL_REF and the MEM.  */
    if (use_object_blocks_p ())
*************** build_constant_desc (tree exp)
*** 3154,3160 ****
  }
  
  /* Return an rtx representing a reference to constant data in memory
!    for the constant expression EXP.
  
     If assembler code for such a constant has already been output,
     return an rtx to refer to it.
--- 3160,3166 ----
  }
  
  /* Return an rtx representing a reference to constant data in memory
!    for the constant expression EXP with the associated DECL.
  
     If assembler code for such a constant has already been output,
     return an rtx to refer to it.
*************** build_constant_desc (tree exp)
*** 3166,3173 ****
  
     `const_desc_table' records which constants already have label strings.  */
  
! rtx
! output_constant_def (tree exp, int defer)
  {
    struct constant_descriptor_tree *desc;
    struct constant_descriptor_tree key;
--- 3172,3179 ----
  
     `const_desc_table' records which constants already have label strings.  */
  
! static rtx
! output_constant_def_1 (tree exp, tree decl, int defer)
  {
    struct constant_descriptor_tree *desc;
    struct constant_descriptor_tree key;
*************** output_constant_def (tree exp, int defer
*** 3182,3188 ****
    desc = (struct constant_descriptor_tree *) *loc;
    if (desc == 0)
      {
!       desc = build_constant_desc (exp);
        desc->hash = key.hash;
        *loc = desc;
      }
--- 3188,3194 ----
    desc = (struct constant_descriptor_tree *) *loc;
    if (desc == 0)
      {
!       desc = build_constant_desc (exp, decl);
        desc->hash = key.hash;
        *loc = desc;
      }
*************** output_constant_def (tree exp, int defer
*** 3191,3196 ****
--- 3197,3211 ----
    return desc->rtl;
  }
  
+ /* Like output_constant_def but create a new decl representing the
+    constant if necessary.  */
+ 
+ rtx
+ output_constant_def (tree exp, int defer)
+ {
+   return  output_constant_def_1  (exp, NULL_TREE, defer);
+ }
+ 
  /* Subroutine of output_constant_def: Decide whether or not we need to
     output the constant DESC now, and if so, do it.  */
  static void
*************** tree_output_constant_def (tree exp)
*** 3327,3333 ****
    desc = (struct constant_descriptor_tree *) *loc;
    if (desc == 0)
      {
!       desc = build_constant_desc (exp);
        desc->hash = key.hash;
        *loc = desc;
      }
--- 3342,3348 ----
    desc = (struct constant_descriptor_tree *) *loc;
    if (desc == 0)
      {
!       desc = build_constant_desc (exp, NULL_TREE);
        desc->hash = key.hash;
        *loc = desc;
      }

Re: [PATCH] Fix PR50494

[Jakub Jelinek]

On Wed, Feb 13, 2013 at 01:04:04PM +0100, Richard Biener wrote:
> 2013-02-13  Richard Biener  <rguenther@suse.de>
> 
> 	PR lto/50494
> 	* varasm.c (output_constant_def_1): Get the decl representing
> 	the constant as argument.
> 	(output_constant_def): Wrap output_constant_def_1.
> 	(make_decl_rtl): Use output_constant_def_1 with the decl
> 	representing the constant.
> 	(build_constant_desc): Optionally re-use a decl already
> 	representing the constant.
> 	(tree_output_constant_def): Adjust.

Looks good to me, except formatting nit:

> + /* Like output_constant_def but create a new decl representing the
> +    constant if necessary.  */
> + 
> + rtx
> + output_constant_def (tree exp, int defer)
> + {
> +   return  output_constant_def_1  (exp, NULL_TREE, defer);

Twice too many spaces.

But I'd say we should also add some varasm.c function that
increase_alignment should call for
(TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
vars upon increasing alignment of the decl, which would do something like:
void
adjust_const_pool_alignment (tree decl)
{
  struct constant_descriptor_tree *desc, key;

  gcc_assert (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl));
  key.value = DECL_INITIAL (decl);
  key.hash = const_hash_1 (DECL_INITIAL (decl));
  desc = (struct constant_descriptor_tree *)
	 htab_find_with_hash (const_desc_htab, &key, key.hash);
  if (desc && MEM_P (desc) && MEM_ALIGN (desc->rtl) < DECL_ALIGN (decl))
    set_mem_align (desc->rtl, DECL_ALIGN (decl));
}
(completely untested).  Because, if we force the constant to be aligned
more than it would be by default, the RTL optimizers should be told about
that too.

	Jakub

Re: [PATCH] Fix PR50494

[Richard Biener]

On Thu, 14 Feb 2013, Jakub Jelinek wrote:

> On Wed, Feb 13, 2013 at 01:04:04PM +0100, Richard Biener wrote:
> > 2013-02-13  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR lto/50494
> > 	* varasm.c (output_constant_def_1): Get the decl representing
> > 	the constant as argument.
> > 	(output_constant_def): Wrap output_constant_def_1.
> > 	(make_decl_rtl): Use output_constant_def_1 with the decl
> > 	representing the constant.
> > 	(build_constant_desc): Optionally re-use a decl already
> > 	representing the constant.
> > 	(tree_output_constant_def): Adjust.
> 
> Looks good to me, except formatting nit:
> 
> > + /* Like output_constant_def but create a new decl representing the
> > +    constant if necessary.  */
> > + 
> > + rtx
> > + output_constant_def (tree exp, int defer)
> > + {
> > +   return  output_constant_def_1  (exp, NULL_TREE, defer);
> 
> Twice too many spaces.
> 
> But I'd say we should also add some varasm.c function that
> increase_alignment should call for
> (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl))
> vars upon increasing alignment of the decl, which would do something like:
> void
> adjust_const_pool_alignment (tree decl)
> {
>   struct constant_descriptor_tree *desc, key;
> 
>   gcc_assert (TREE_CODE (decl) == VAR_DECL && DECL_IN_CONSTANT_POOL (decl));
>   key.value = DECL_INITIAL (decl);
>   key.hash = const_hash_1 (DECL_INITIAL (decl));
>   desc = (struct constant_descriptor_tree *)
> 	 htab_find_with_hash (const_desc_htab, &key, key.hash);
>   if (desc && MEM_P (desc) && MEM_ALIGN (desc->rtl) < DECL_ALIGN (decl))
>     set_mem_align (desc->rtl, DECL_ALIGN (decl));
> }
> (completely untested).  Because, if we force the constant to be aligned
> more than it would be by default, the RTL optimizers should be told about
> that too.

At the moment the vectorizer refuses to bump alignment for anything
with TREE_ASM_WRITTEN set.  I suppose that should rather be checking
DECL_RTL_SET_P.  Btw, isn't the constants descriptor ->rtl always
that of DECL_RTL of the decl?

But yes, we should have a proper abstraction for can_adjust_alignment
and adjust_alignment.  But we should use that for all decls, not
just DECL_IN_CONSTANT_POOL.

Thanks,
Richard.

Re: [PATCH] Fix PR50494

[Jakub Jelinek]

On Thu, Feb 14, 2013 at 12:53:47PM +0100, Richard Biener wrote:
> > (completely untested).  Because, if we force the constant to be aligned
> > more than it would be by default, the RTL optimizers should be told about
> > that too.
> 
> At the moment the vectorizer refuses to bump alignment for anything
> with TREE_ASM_WRITTEN set.  I suppose that should rather be checking
> DECL_RTL_SET_P.  Btw, isn't the constants descriptor ->rtl always
> that of DECL_RTL of the decl?

But at that point the decls aren't TREE_ASM_WRITTEN, and don't have
DECL_RTL (otherwise, it would be enough to bump alignment of DECL_RTL).
The rtl is nevertheless already created at that point, by
build_constant_desc (in this case called by tree_output_constant_def).

	Jakub