Fix increase_alignment

Fix increase_alignment

[Jan Hubicka]

Hi,
this patch makes vectorizer to increase alignments correctly in presence
of aliases.  Currently increase happens at two places, first in
increase_alignment where we increase alignment of the DECL of alias as well
as alias target.   This is not safe: it is possible that alias is static
and binds to current defintion, while target is not and may be interposed.
In this case we can not increase DECL_ALIGNMENT of target.

In the other place only alias DECL_ALIGN is bumped up. this however does not
make the definition (of alias target) to come out aligned and leads to segfault
in the testcase.

This patch moves the alignment decision to symbol table.
node->definition_alignment returns the desired alignment of the defintiion that
is maximal alignment needed by the declarations of symbol and its aliases.

I also moved logic deciding how to increase alignment into
node->can_increase_alignment_p predicate and node->increase_alignment that
increases alignment of all aliases that binds to current def.

I unforutnately spot another problem. With presence of section anchors we
can not increase alignment of symbol that was already placed into the section.
I think this is reason why we still have pass_increase_alignments that runs
only on target with anchors.

I added the following check:

+  /* If target is already placed in an anchor, we can not touch its
+     alignment.  */
+  if (DECL_RTL_SET_P (target->decl)
+      && MEM_P (DECL_RTL (target->decl))
+      && SYMBOL_REF_HAS_BLOCK_INFO_P (XEXP (DECL_RTL (target->decl), 0)))
+    return false;

but found it breaks several vectorization testcases with LTO.  This is
becuase notice_global_symbol computes DECL_RTL even before 
node->increase_alignment is run and we are stuck.

I decided to track this separately from this patch that snowballed quite a bit
already. I do not see how to solve it short of making notice_global_symbol to
not compute DECL_RTL.  It was never quite clear to me why notice_global_symbol
is actually doing this at first place.  Is it becuase of target mangling
happing only at RTL generation time?

Bootstrapped/regtested x86_64-linux, ppc64-linux, bootstrapped on AIX, will
commit it later today.

Honza
	PR ipa/65334
	* cgraph.h (symtab_node): Add definition_alignment,
	can_increase_alignment_p and increase_alignment.
	* symtab.c (symtab_node::can_increase_alignment_p,
	increase_alignment_1, symtab_node::increase_alignment,
	symtab_node::definition_alignment): New.
	* tree-vect-data-refs.c (vect_can_force_dr_alignment_p): Use
	can_increase_alignment_p.
	* tree-vectorizer.c (increase_alignment): Use increase_alignment.
	* tree-vect-stmts.c (ensure_base_align): Likewise.
Index: tree-vect-data-refs.c
===================================================================
--- tree-vect-data-refs.c	(revision 221223)
+++ tree-vect-data-refs.c	(working copy)
@@ -5703,58 +5703,10 @@ vect_can_force_dr_alignment_p (const_tre
   if (TREE_CODE (decl) != VAR_DECL)
     return false;
 
-  /* With -fno-toplevel-reorder we may have already output the constant.  */
-  if (TREE_ASM_WRITTEN (decl))
+  if (decl_in_symtab_p (decl)
+      && !symtab_node::get (decl)->can_increase_alignment_p ())
     return false;
 
-  /* Constant pool entries may be shared and not properly merged by LTO.  */
-  if (DECL_IN_CONSTANT_POOL (decl))
-    return false;
-
-  if (TREE_PUBLIC (decl) || DECL_EXTERNAL (decl))
-    {
-      symtab_node *snode;
-
-      /* We cannot change alignment of symbols that may bind to symbols
-	 in other translation unit that may contain a definition with lower
-	 alignment.  */
-      if (!decl_binds_to_current_def_p (decl))
-	return false;
-
-      /* When compiling partition, be sure the symbol is not output by other
-	 partition.  */
-      snode = symtab_node::get (decl);
-      if (flag_ltrans
-	  && (snode->in_other_partition
-	      || snode->get_partitioning_class () == SYMBOL_DUPLICATE))
-	return false;
-    }
-
-  /* Do not override the alignment as specified by the ABI when the used
-     attribute is set.  */
-  if (DECL_PRESERVE_P (decl))
-    return false;
-
-  /* Do not override explicit alignment set by the user when an explicit
-     section name is also used.  This is a common idiom used by many
-     software projects.  */
-  if (TREE_STATIC (decl) 
-      && DECL_SECTION_NAME (decl) != NULL
-      && !symtab_node::get (decl)->implicit_section)
-    return false;
-
-  /* If symbol is an alias, we need to check that target is OK.  */
-  if (TREE_STATIC (decl))
-    {
-      tree target = symtab_node::get (decl)->ultimate_alias_target ()->decl;
-      if (target != decl)
-	{
-	  if (DECL_PRESERVE_P (target))
-	    return false;
-	  decl = target;
-	}
-    }
-
   if (TREE_STATIC (decl))
     return (alignment <= MAX_OFILE_ALIGNMENT);
   else
Index: symtab.c
===================================================================
--- symtab.c	(revision 221223)
+++ symtab.c	(working copy)
@@ -1908,3 +1908,99 @@ symtab_node::address_matters_p ()
   gcc_assert (!alias);
   return call_for_symbol_and_aliases (address_matters_1, NULL, true);
 }
+
+/* Return ture if symbol's alignment may be increased.  */
+
+bool
+symtab_node::can_increase_alignment_p (void)
+{
+  symtab_node *target = ultimate_alias_target ();
+
+  /* For now support only variables.  */
+  if (TREE_CODE (decl) != VAR_DECL)
+    return false;
+
+  /* With -fno-toplevel-reorder we may have already output the constant.  */
+  if (TREE_ASM_WRITTEN (target->decl))
+    return false;
+
+  /* Constant pool entries may be shared.  */
+  if (DECL_IN_CONSTANT_POOL (target->decl))
+    return false;
+
+  /* We cannot change alignment of symbols that may bind to symbols
+     in other translation unit that may contain a definition with lower
+     alignment.  */
+  if (!decl_binds_to_current_def_p (decl))
+    return false;
+
+  /* When compiling partition, be sure the symbol is not output by other
+     partition.  */
+  if (flag_ltrans
+      && (target->in_other_partition
+	  || target->get_partitioning_class () == SYMBOL_DUPLICATE))
+    return false;
+
+  /* Do not override the alignment as specified by the ABI when the used
+     attribute is set.  */
+  if (DECL_PRESERVE_P (decl) || DECL_PRESERVE_P (target->decl))
+    return false;
+
+  /* Do not override explicit alignment set by the user when an explicit
+     section name is also used.  This is a common idiom used by many
+     software projects.  */
+  if (DECL_SECTION_NAME (target->decl) != NULL && !target->implicit_section)
+    return false;
+
+  return true;
+}
+
+/* Worker for symtab_node::increase_alignment.  */
+
+static bool
+increase_alignment_1 (symtab_node *n, void *v)
+{
+  unsigned int align = (size_t)v;
+  if (DECL_ALIGN (n->decl) < align
+      && n->can_increase_alignment_p ())
+    {
+      DECL_ALIGN (n->decl) = align;
+      DECL_USER_ALIGN (n->decl) = 1;
+    }
+  return false;
+}
+
+/* Increase alignment of THIS to ALIGN.  */
+
+void
+symtab_node::increase_alignment (unsigned int align)
+{
+  gcc_assert (can_increase_alignment_p () && align < MAX_OFILE_ALIGNMENT);
+  ultimate_alias_target()->call_for_symbol_and_aliases (increase_alignment_1,
+						        (void *)(size_t) align,
+						        true);
+  gcc_assert (DECL_ALIGN (decl) >= align);
+}
+
+/* Helper for symtab_node::definition_alignment.  */
+
+static bool
+get_alignment_1 (symtab_node *n, void *v)
+{
+  *((unsigned int *)v) = MAX (*((unsigned int *)v), DECL_ALIGN (n->decl));
+  return false;
+}
+
+/* Return desired alignment of the definition.  This is NOT alignment useful
+   to access THIS, because THIS may be interposable and DECL_ALIGN should
+   be used instead.  It however must be guaranteed when output definition
+   of THIS.  */
+
+unsigned int
+symtab_node::definition_alignment ()
+{
+  unsigned int align = 0;
+  gcc_assert (!alias);
+  call_for_symbol_and_aliases (get_alignment_1, &align, true);
+  return align;
+}
Index: tree-vectorizer.c
===================================================================
--- tree-vectorizer.c	(revision 221223)
+++ tree-vectorizer.c	(working copy)
@@ -719,14 +719,7 @@ increase_alignment (void)
 
       if (vect_can_force_dr_alignment_p (decl, alignment))
         {
-          DECL_ALIGN (decl) = TYPE_ALIGN (vectype);
-          DECL_USER_ALIGN (decl) = 1;
-	  if (TREE_STATIC (decl))
-	    {
-	      tree target = symtab_node::get (decl)->ultimate_alias_target ()->decl;
-              DECL_ALIGN (target) = TYPE_ALIGN (vectype);
-              DECL_USER_ALIGN (target) = 1;
-	    }
+	  vnode->increase_alignment (TYPE_ALIGN (vectype));
           dump_printf (MSG_NOTE, "Increasing alignment of decl: ");
           dump_generic_expr (MSG_NOTE, TDF_SLIM, decl);
           dump_printf (MSG_NOTE, "\n");
Index: cgraph.h
===================================================================
--- cgraph.h	(revision 221223)
+++ cgraph.h	(working copy)
@@ -289,6 +289,18 @@ public:
   /* Make DECL local.  */
   void make_decl_local (void);
 
+  /* Return desired alignment of the definition.  This is NOT alignment useful
+     to access THIS, because THIS may be interposable and DECL_ALIGN should
+     be used instead.  It however must be guaranteed when output definition
+     of THIS.  */
+  unsigned int definition_alignment ();
+
+  /* Return true if alignment can be increased.  */
+  bool can_increase_alignment_p ();
+
+  /* Increase alignment of symbol to ALIGN.  */
+  void increase_alignment (unsigned int align);
+
   /* Return true if list contains an alias.  */
   bool has_aliases_p (void);
 
Index: tree-vect-stmts.c
===================================================================
--- tree-vect-stmts.c	(revision 221223)
+++ tree-vect-stmts.c	(working copy)
@@ -4956,8 +4956,13 @@ ensure_base_align (stmt_vec_info stmt_in
       tree vectype = STMT_VINFO_VECTYPE (stmt_info);
       tree base_decl = ((dataref_aux *)dr->aux)->base_decl;
 
-      DECL_ALIGN (base_decl) = TYPE_ALIGN (vectype);
-      DECL_USER_ALIGN (base_decl) = 1;
+      if (decl_in_symtab_p (base_decl))
+	symtab_node::get (base_decl)->increase_alignment (TYPE_ALIGN (vectype));
+      else
+	{
+          DECL_ALIGN (base_decl) = TYPE_ALIGN (vectype);
+          DECL_USER_ALIGN (base_decl) = 1;
+	}
       ((dataref_aux *)dr->aux)->base_misaligned = false;
     }
 }

Re: Fix increase_alignment

[Jan Hubicka]

> 
> I unforutnately spot another problem. With presence of section anchors we
> can not increase alignment of symbol that was already placed into the section.
> I think this is reason why we still have pass_increase_alignments that runs
> only on target with anchors.
> 
> I added the following check:
> 
> +  /* If target is already placed in an anchor, we can not touch its
> +     alignment.  */
> +  if (DECL_RTL_SET_P (target->decl)
> +      && MEM_P (DECL_RTL (target->decl))
> +      && SYMBOL_REF_HAS_BLOCK_INFO_P (XEXP (DECL_RTL (target->decl), 0)))
> +    return false;
> 
> but found it breaks several vectorization testcases with LTO.  This is
> becuase notice_global_symbol computes DECL_RTL even before 
> node->increase_alignment is run and we are stuck.
> 
> I decided to track this separately from this patch that snowballed quite a bit
> already. I do not see how to solve it short of making notice_global_symbol to
> not compute DECL_RTL.  It was never quite clear to me why notice_global_symbol
> is actually doing this at first place.  Is it becuase of target mangling
> happing only at RTL generation time?

This is now PR65355

Re: Fix increase_alignment

[Bernhard Reutner-Fischer]

On 9 March 2015 at 00:37, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> I unforutnately spot another problem. With presence of section anchors we
>> can not increase alignment of symbol that was already placed into the section.
>> I think this is reason why we still have pass_increase_alignments that runs
>> only on target with anchors.
>>
>> I added the following check:
>>
>> +  /* If target is already placed in an anchor, we can not touch its
>> +     alignment.  */
>> +  if (DECL_RTL_SET_P (target->decl)
>> +      && MEM_P (DECL_RTL (target->decl))
>> +      && SYMBOL_REF_HAS_BLOCK_INFO_P (XEXP (DECL_RTL (target->decl), 0)))
>> +    return false;
>>
>> but found it breaks several vectorization testcases with LTO.  This is
>> becuase notice_global_symbol computes DECL_RTL even before
>> node->increase_alignment is run and we are stuck.
>>
>> I decided to track this separately from this patch that snowballed quite a bit
>> already. I do not see how to solve it short of making notice_global_symbol to
>> not compute DECL_RTL.  It was never quite clear to me why notice_global_symbol
>> is actually doing this at first place.  Is it becuase of target mangling
>> happing only at RTL generation time?
>
> This is now PR65355


Your follow-up patch 88ada5e935d58223ae2d9ce6d0c1c71c372680a8 a.k.a r221269
added this to emit_local():

 static bool
-emit_local (tree decl ATTRIBUTE_UNUSED,
+emit_local (tree decl,
            const char *name ATTRIBUTE_UNUSED,
            unsigned HOST_WIDE_INT size ATTRIBUTE_UNUSED,
            unsigned HOST_WIDE_INT rounded ATTRIBUTE_UNUSED)
 {
+  int align = symtab_node::get (decl)->definition_alignment ();
 #if defined ASM_OUTPUT_ALIGNED_DECL_LOCAL
   ASM_OUTPUT_ALIGNED_DECL_LOCAL (asm_out_file, decl, name,
-                                size, DECL_ALIGN (decl));
+                                size, align);
   return true;
 #elif defined ASM_OUTPUT_ALIGNED_LOCAL
-  ASM_OUTPUT_ALIGNED_LOCAL (asm_out_file, name, size, DECL_ALIGN (decl));
+  ASM_OUTPUT_ALIGNED_LOCAL (asm_out_file, name, size, align);
   return true;
 #else
   ASM_OUTPUT_LOCAL (asm_out_file, name, size, rounded);
   return false;
 #endif
 }

which gives

gcc/varasm.c:1936:7: error: unused variable ‘align’ [-Werror=unused-variable]
   int align = symtab_node::get (decl)->definition_alignment ();
       ^

on
log/alpha64-dec-vms
log/alpha-dec-vms
log/i686-cygwinOPT-enable-threads=yes
log/i686-mingw32crt
log/i686-openbsd3.0
log/i686-pc-msdosdjgpp
log/m68k-openbsd

Maybe just flag it as used or copy-move it?

TIA,

Re: Fix increase_alignment

[Jan Hubicka]

> 
> Your follow-up patch 88ada5e935d58223ae2d9ce6d0c1c71c372680a8 a.k.a r221269
> added this to emit_local():
> 
>  static bool
> -emit_local (tree decl ATTRIBUTE_UNUSED,
> +emit_local (tree decl,
>             const char *name ATTRIBUTE_UNUSED,
>             unsigned HOST_WIDE_INT size ATTRIBUTE_UNUSED,
>             unsigned HOST_WIDE_INT rounded ATTRIBUTE_UNUSED)
>  {
> +  int align = symtab_node::get (decl)->definition_alignment ();
>  #if defined ASM_OUTPUT_ALIGNED_DECL_LOCAL
>    ASM_OUTPUT_ALIGNED_DECL_LOCAL (asm_out_file, decl, name,
> -                                size, DECL_ALIGN (decl));
> +                                size, align);
>    return true;
>  #elif defined ASM_OUTPUT_ALIGNED_LOCAL
> -  ASM_OUTPUT_ALIGNED_LOCAL (asm_out_file, name, size, DECL_ALIGN (decl));
> +  ASM_OUTPUT_ALIGNED_LOCAL (asm_out_file, name, size, align);
>    return true;
>  #else
>    ASM_OUTPUT_LOCAL (asm_out_file, name, size, rounded);
>    return false;
>  #endif
>  }
> 
> which gives
> 
> gcc/varasm.c:1936:7: error: unused variable ‘align’ [-Werror=unused-variable]
>    int align = symtab_node::get (decl)->definition_alignment ();
>        ^
> 
> on
> log/alpha64-dec-vms
> log/alpha-dec-vms
> log/i686-cygwinOPT-enable-threads=yes
> log/i686-mingw32crt
> log/i686-openbsd3.0
> log/i686-pc-msdosdjgpp
> log/m68k-openbsd
> 
> Maybe just flag it as used or copy-move it?

Yep, lets just move it into the ifdefs. Can you please check that the alignment
looks right atone of those targets? I am not quite sure who is supposed to
do so on targets not defining ASM_OUTPUT_ALIGNED_LOCAL.

Perhaps we want then to prvent vectorizer from updating the alignments.

Honza
> 
> TIA,

Re: Fix increase_alignment

[Bernhard Reutner-Fischer]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On 2 April 2015 at 01:20, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> Your follow-up patch 88ada5e935d58223ae2d9ce6d0c1c71c372680a8 a.k.a r221269
>> added this to emit_local():
>>
>>  static bool
>> -emit_local (tree decl ATTRIBUTE_UNUSED,
>> +emit_local (tree decl,
>>             const char *name ATTRIBUTE_UNUSED,
>>             unsigned HOST_WIDE_INT size ATTRIBUTE_UNUSED,
>>             unsigned HOST_WIDE_INT rounded ATTRIBUTE_UNUSED)
>>  {
>> +  int align = symtab_node::get (decl)->definition_alignment ();
>>  #if defined ASM_OUTPUT_ALIGNED_DECL_LOCAL
>>    ASM_OUTPUT_ALIGNED_DECL_LOCAL (asm_out_file, decl, name,
>> -                                size, DECL_ALIGN (decl));
>> +                                size, align);
>>    return true;
>>  #elif defined ASM_OUTPUT_ALIGNED_LOCAL
>> -  ASM_OUTPUT_ALIGNED_LOCAL (asm_out_file, name, size, DECL_ALIGN (decl));
>> +  ASM_OUTPUT_ALIGNED_LOCAL (asm_out_file, name, size, align);
>>    return true;
>>  #else
>>    ASM_OUTPUT_LOCAL (asm_out_file, name, size, rounded);
>>    return false;
>>  #endif
>>  }
>>
>> which gives
>>
>> gcc/varasm.c:1936:7: error: unused variable ‘align’ [-Werror=unused-variable]
>>    int align = symtab_node::get (decl)->definition_alignment ();
>>        ^
>>
>> on
>> log/alpha64-dec-vms
>> log/alpha-dec-vms
>> log/i686-cygwinOPT-enable-threads=yes
>> log/i686-mingw32crt
>> log/i686-openbsd3.0
>> log/i686-pc-msdosdjgpp
>> log/m68k-openbsd
>>
>> Maybe just flag it as used or copy-move it?
>
> Yep, lets just move it into the ifdefs. Can you please check that the alignment

Committed revision 221925.

> looks right atone of those targets? I am not quite sure who is supposed to
> do so on targets not defining ASM_OUTPUT_ALIGNED_LOCAL.
>
> Perhaps we want then to prvent vectorizer from updating the alignments.

I have no idea how the alignment on those targets are supposed to look
like, sorry.

thanks,

[-- Attachment #2: gcc-r221925.patch --]
[-- Type: text/x-patch, Size: 2059 bytes --]

------------------------------------------------------------------------
r221925 | aldot | 2015-04-08 19:56:18 +0200 (Wed, 08 Apr 2015) | 22 lines

emit_local(): Fix unused warning

Honzas r221269 produced

gcc/varasm.c:1936:7: error: unused variable ‘align’
[-Werror=unused-variable]
   int align = symtab_node::get (decl)->definition_alignment ();
       ^
on e.g.:

log/alpha64-dec-vms
log/alpha-dec-vms
log/i686-cygwinOPT-enable-threads=yes
log/i686-mingw32crt
log/i686-openbsd3.0
log/i686-pc-msdosdjgpp
log/m68k-openbsd

Silence this by moving the variable into the corresponding blocks and
adding back the ATTRIBUTE_UNUSED decoration for the decl param.


------------------------------------------------------------------------
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 221924)
+++ gcc/ChangeLog	(revision 221925)
@@ -1,3 +1,7 @@
+2015-04-08  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>
+
+	* varasm.c (emit_local): Move definition of align.
+
 2015-04-08  Julian Brown  <julian@codesourcery.com>
 
 	* config/nvptx/mkoffload.c (process): Support variable mapping.
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 221924)
+++ gcc/varasm.c	(revision 221925)
@@ -1928,17 +1928,18 @@
 /* A noswitch_section_callback for lcomm_section.  */
 
 static bool
-emit_local (tree decl,
+emit_local (tree decl ATTRIBUTE_UNUSED,
 	    const char *name ATTRIBUTE_UNUSED,
 	    unsigned HOST_WIDE_INT size ATTRIBUTE_UNUSED,
 	    unsigned HOST_WIDE_INT rounded ATTRIBUTE_UNUSED)
 {
+#if defined ASM_OUTPUT_ALIGNED_DECL_LOCAL
   int align = symtab_node::get (decl)->definition_alignment ();
-#if defined ASM_OUTPUT_ALIGNED_DECL_LOCAL
   ASM_OUTPUT_ALIGNED_DECL_LOCAL (asm_out_file, decl, name,
 				 size, align);
   return true;
 #elif defined ASM_OUTPUT_ALIGNED_LOCAL
+  int align = symtab_node::get (decl)->definition_alignment ();
   ASM_OUTPUT_ALIGNED_LOCAL (asm_out_file, name, size, align);
   return true;
 #else