diff mbox

Add a new type attribute always_alias (PR79671)

Message ID alpine.LSU.2.20.1704111226090.30715@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener April 11, 2017, 10:32 a.m. UTC 
On Mon, 10 Apr 2017, Jason Merrill wrote:

> On Mon, Apr 10, 2017 at 11:30 AM, Richard Biener <rguenther@suse.de> wrote:
> > On Mon, 10 Apr 2017, Jason Merrill wrote:
> >> On Mon, Apr 10, 2017 at 8:50 AM, Richard Biener <rguenther@suse.de> wrote:
> >> >         * tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE
> >> >         for arrays of unsigned char or std::byte.
> >>
> >> I think it would be good to have a flag to select whether these
> >> semantics apply to any char variant and std::byte, only unsigned char
> >> and std::byte, or only std::byte.
> >
> > Any suggestion?  Not sure we really need it (I'm hesitant to write
> > all the testcases to verify it actually works).
> 
> Well, there's existing code that uses plain char (e.g. boost) that I
> want to support.  If there's a significant optimization penalty for
> allowing that, we probably also want to be able to limit the impact.
> If there isn't much penalty, let's just support all char variants.

I haven't assessed the penalty involved but it can't be worse than
the situation we had in GCC 6.  So I think it's reasonable to support
all char variants for now.  One could add some instrumenting to
alias_set_subset_of / alias_sets_conflict_p but it would only yield
an upper bound on the number of failed queries (TBAA is a quite early
out before PTA info is used for example).

The following variant -- added missing

Comments

Richard Biener April 11, 2017, 11:53 a.m. UTC | #1 
On Tue, 11 Apr 2017, Richard Biener wrote:

> On Mon, 10 Apr 2017, Jason Merrill wrote:
> 
> > On Mon, Apr 10, 2017 at 11:30 AM, Richard Biener <rguenther@suse.de> wrote:
> > > On Mon, 10 Apr 2017, Jason Merrill wrote:
> > >> On Mon, Apr 10, 2017 at 8:50 AM, Richard Biener <rguenther@suse.de> wrote:
> > >> >         * tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE
> > >> >         for arrays of unsigned char or std::byte.
> > >>
> > >> I think it would be good to have a flag to select whether these
> > >> semantics apply to any char variant and std::byte, only unsigned char
> > >> and std::byte, or only std::byte.
> > >
> > > Any suggestion?  Not sure we really need it (I'm hesitant to write
> > > all the testcases to verify it actually works).
> > 
> > Well, there's existing code that uses plain char (e.g. boost) that I
> > want to support.  If there's a significant optimization penalty for
> > allowing that, we probably also want to be able to limit the impact.
> > If there isn't much penalty, let's just support all char variants.
> 
> I haven't assessed the penalty involved but it can't be worse than
> the situation we had in GCC 6.  So I think it's reasonable to support
> all char variants for now.  One could add some instrumenting to
> alias_set_subset_of / alias_sets_conflict_p but it would only yield
> an upper bound on the number of failed queries (TBAA is a quite early
> out before PTA info is used for example).
> 
> The following variant -- added missing
> 
> Index: gcc/cp/tree.c
> ===================================================================
> --- gcc/cp/tree.c       (revision 246832)
> +++ gcc/cp/tree.c       (working copy)
> @@ -972,6 +979,7 @@ build_cplus_array_type (tree elt_type, t
>                  as it will overwrite alignment etc. of all variants.  */
>               TYPE_SIZE (t) = TYPE_SIZE (m);
>               TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
> +             TYPE_TYPELESS_STORAGE (t) = TYPE_TYPELESS_STORAGE (m);
>             }
>  
>           TYPE_MAIN_VARIANT (t) = m;
> 
> that caused LTO bootstrap to fail and removed the tree-ssa-structalias.c
> change (committed separately) [LTO] bootstrapped and tested ok on 
> x86_64-unknown-linux-gnu.
> 
> I've tested some template examples and they seem to work fine.
> 
> Ok for trunk?
> 
> Disclaimer: there might still be an issue with cross-language LTO
> support, but it might at most result in TYPE_TYPELESS_STORAGE
> getting lost.  Trying to craft a testcase to verify my theory.

Not too difficult in the end (see below).  A fix (below) is to
not treat arrays with differing TYPE_TYPELESS_STORAGE as
compatible for the purpose of computing TYPE_CANONICAL (and
thus recursively structures containing them).  While they'd
still alias each other (because currently a TYPE_TYPELESS_STORAGE
member makes the whole thing effectively alias anything) this
results in warnings in case such a type is used in the interface
between C and C++ (it's also considered a ODR type).

t.C:17:17: warning: type of ‘bar’ does not match original declaration 
[-Wlto-type-mismatch]
 extern "C" void bar (X *);
                 ^
t2.c:5:6: note: ‘bar’ was previously declared here
 void bar (struct X *p) {}
      ^

if you add a struct X * parameter to bar().

So it's not the optimal solution here.  Another fix would be to
somehow make TYPE_TYPELESS_STORAGE "sticky" when merging canonical
types but there's not precedent in doing this kind of thing and
I'm not sure we'll get everything merged before computing alias
sets.

CCing Honza.

Richard.

2017-04-11  Richard Biener  <rguenther@suse.de>

	PR middle-end/79671
	* tree.c (gimple_canonical_types_compatible_p): Do not treat
	arrays with differing TYPE_TYPELESS_STORAGE as compatible.

	* g++.dg/lto/pr79671_0.C: New testcase.
	* g++.dg/lto/pr79671_1.c: Likewise.

Index: gcc/tree.c
===================================================================
*** gcc/tree.c	(revision 246835)
--- gcc/tree.c	(working copy)
*************** gimple_canonical_types_compatible_p (con
*** 13709,13715 ****
  						trust_type_canonical)
  	  || TYPE_STRING_FLAG (t1) != TYPE_STRING_FLAG (t2)
  	  || TYPE_REVERSE_STORAGE_ORDER (t1) != TYPE_REVERSE_STORAGE_ORDER (t2)
! 	  || TYPE_NONALIASED_COMPONENT (t1) != TYPE_NONALIASED_COMPONENT (t2))
  	return false;
        else
  	{
--- 13709,13716 ----
  						trust_type_canonical)
  	  || TYPE_STRING_FLAG (t1) != TYPE_STRING_FLAG (t2)
  	  || TYPE_REVERSE_STORAGE_ORDER (t1) != TYPE_REVERSE_STORAGE_ORDER (t2)
! 	  || TYPE_NONALIASED_COMPONENT (t1) != TYPE_NONALIASED_COMPONENT (t2)
! 	  || TYPE_TYPELESS_STORAGE (t1) != TYPE_TYPELESS_STORAGE (t2))
  	return false;
        else
  	{
Index: gcc/testsuite/g++.dg/lto/pr79671_0.C
===================================================================
*** gcc/testsuite/g++.dg/lto/pr79671_0.C	(nonexistent)
--- gcc/testsuite/g++.dg/lto/pr79671_0.C	(working copy)
***************
*** 0 ****
--- 1,26 ----
+ // { dg-lto-do run }
+ 
+ void *operator new(__SIZE_TYPE__, void *p2) { return p2; }
+ struct B { B(int i_) : i(i_) {} int i; };
+ struct X
+ {
+   unsigned char buf[sizeof (B)];
+ };
+ 
+ int __attribute__((noinline)) foo()
+ {
+   X x alignas (B), y alignas (B);
+   new (&x) B (0);
+   y = x;
+   B *q = reinterpret_cast <B *>(&y);
+   asm volatile ("" : "=r" (q) : "r" (q));
+   return q->i;
+ }
+ extern "C" void bar ();
+ int main()
+ {
+   if (foo() != 0)
+     __builtin_abort ();
+   bar ();
+   return 0;
+ }
Index: gcc/testsuite/g++.dg/lto/pr79671_1.c
===================================================================
*** gcc/testsuite/g++.dg/lto/pr79671_1.c	(nonexistent)
--- gcc/testsuite/g++.dg/lto/pr79671_1.c	(working copy)
***************
*** 0 ****
--- 1,5 ----
+ struct X
+ {
+   unsigned char buf[sizeof (int)];
+ };
+ void bar () { struct X x; *(volatile char *)x.buf = 1; }
diff mbox

Patch

Index: gcc/cp/tree.c
===================================================================
--- gcc/cp/tree.c       (revision 246832)
+++ gcc/cp/tree.c       (working copy)
@@ -972,6 +979,7 @@  build_cplus_array_type (tree elt_type, t
                 as it will overwrite alignment etc. of all variants.  */
              TYPE_SIZE (t) = TYPE_SIZE (m);
              TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
+             TYPE_TYPELESS_STORAGE (t) = TYPE_TYPELESS_STORAGE (m);
            }
 
          TYPE_MAIN_VARIANT (t) = m;

that caused LTO bootstrap to fail and removed the tree-ssa-structalias.c
change (committed separately) [LTO] bootstrapped and tested ok on 
x86_64-unknown-linux-gnu.

I've tested some template examples and they seem to work fine.

Ok for trunk?

Disclaimer: there might still be an issue with cross-language LTO
support, but it might at most result in TYPE_TYPELESS_STORAGE
getting lost.  Trying to craft a testcase to verify my theory.

Thanks,
Richard.

2017-04-11  Richard Biener  <rguenther@suse.de>

	PR c++/79671
	cp/
	* tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE
	for arrays of unsigned char or std::byte.

	* tree-core.h (tree_type_common): Add typeless_storage flag.
	* tree.h (TYPE_TYPELESS_STORAGE): New macro.
	(canonical_type_used_p): Add arrays with TYPE_TYPELESS_STORAGE.
	* alias.c (alias_set_entry): Add has_typeless_storage member.
	(alias_set_subset_of): Handle it.
	(alias_sets_conflict_p): Likewise.
	(init_alias_set_entry): Initialize it.
	(get_alias_set): For ARRAY_TYPEs handle TYPE_TYPELESS_STORAGE.
	(record_alias_subset): Likewise.
	* lto-streamer-out.c (hash_tree): Hash TYPE_TYPELESS_STORAGE.
	* tree-streamer-in.c (unpack_ts_type_common_value_fields): Stream
	TYPE_TYPELESS_STORAGE.
	* tree-streamer-out.c (pack_ts_type_common_value_fields): Likewise.

	lto/
	* lto.c (compare_tree_sccs_1): Compare TYPE_TYPELESS_STORAGE.

	* g++.dg/torture/pr79671.C: New testcase.

Index: gcc/alias.c
===================================================================
--- gcc/alias.c	(revision 246832)
+++ gcc/alias.c	(working copy)
@@ -136,6 +136,9 @@  struct GTY(()) alias_set_entry {
   bool is_pointer;
   /* Nonzero if is_pointer or if one of childs have has_pointer set.  */
   bool has_pointer;
+  /* Nonzero if we have a child serving as typeless storage (or are
+     such storage ourselves).  */
+  bool has_typeless_storage;
 
   /* The children of the alias set.  These are not just the immediate
      children, but, in fact, all descendants.  So, if we have:
@@ -419,7 +422,8 @@  alias_set_subset_of (alias_set_type set1
   /* Check if set1 is a subset of set2.  */
   ase2 = get_alias_set_entry (set2);
   if (ase2 != 0
-      && (ase2->has_zero_child
+      && (ase2->has_typeless_storage
+	  || ase2->has_zero_child
 	  || (ase2->children && ase2->children->get (set1))))
     return true;
 
@@ -480,7 +484,8 @@  alias_sets_conflict_p (alias_set_type se
   /* See if the first alias set is a subset of the second.  */
   ase1 = get_alias_set_entry (set1);
   if (ase1 != 0
-      && ase1->children && ase1->children->get (set2))
+      && (ase1->has_typeless_storage
+	  || (ase1->children && ase1->children->get (set2))))
     {
       ++alias_stats.num_dag;
       return 1;
@@ -489,7 +494,8 @@  alias_sets_conflict_p (alias_set_type se
   /* Now do the same, but with the alias sets reversed.  */
   ase2 = get_alias_set_entry (set2);
   if (ase2 != 0
-      && ase2->children && ase2->children->get (set1))
+      && (ase2->has_typeless_storage
+	  || (ase2->children && ase2->children->get (set1))))
     {
       ++alias_stats.num_dag;
       return 1;
@@ -825,6 +831,7 @@  init_alias_set_entry (alias_set_type set
   ase->has_zero_child = false;
   ase->is_pointer = false;
   ase->has_pointer = false;
+  ase->has_typeless_storage = false;
   gcc_checking_assert (!get_alias_set_entry (set));
   (*alias_sets)[set] = ase;
   return ase;
@@ -955,6 +962,7 @@  get_alias_set (tree t)
      Just be pragmatic here and make sure the array and its element
      type get the same alias set assigned.  */
   else if (TREE_CODE (t) == ARRAY_TYPE
+	   && ! TYPE_TYPELESS_STORAGE (t)
 	   && (!TYPE_NONALIASED_COMPONENT (t)
 	       || TYPE_STRUCTURAL_EQUALITY_P (t)))
     set = get_alias_set (TREE_TYPE (t));
@@ -1094,6 +1102,15 @@  get_alias_set (tree t)
 
   TYPE_ALIAS_SET (t) = set;
 
+  if (TREE_CODE (t) == ARRAY_TYPE
+      && TYPE_TYPELESS_STORAGE (t))
+    {
+      alias_set_entry *ase = get_alias_set_entry (set);
+      if (!ase)
+	ase = init_alias_set_entry (set);
+      ase->has_typeless_storage = true;
+    }
+
   /* If this is an aggregate type or a complex type, we must record any
      component aliasing information.  */
   if (AGGREGATE_TYPE_P (t) || TREE_CODE (t) == COMPLEX_TYPE)
@@ -1173,6 +1190,8 @@  record_alias_subset (alias_set_type supe
 	    superset_entry->has_zero_child = true;
           if (subset_entry->has_pointer)
 	    superset_entry->has_pointer = true;
+	  if (subset_entry->has_typeless_storage)
+	    superset_entry->has_typeless_storage = true;
 
 	  if (subset_entry->children)
 	    {
Index: gcc/cp/tree.c
===================================================================
--- gcc/cp/tree.c	(revision 246832)
+++ gcc/cp/tree.c	(working copy)
@@ -949,6 +949,13 @@  build_cplus_array_type (tree elt_type, t
   else
     {
       t = build_array_type (elt_type, index_type);
+      if (elt_type == unsigned_char_type_node
+	  || elt_type == signed_char_type_node
+	  || elt_type == char_type_node
+	  || (TREE_CODE (elt_type) == ENUMERAL_TYPE
+	      && TYPE_CONTEXT (elt_type) == std_node
+	      && !strcmp ("byte", TYPE_NAME_STRING (elt_type))))
+	TYPE_TYPELESS_STORAGE (t) = 1;
     }
 
   /* Now check whether we already have this array variant.  */
@@ -972,6 +979,7 @@  build_cplus_array_type (tree elt_type, t
 		 as it will overwrite alignment etc. of all variants.  */
 	      TYPE_SIZE (t) = TYPE_SIZE (m);
 	      TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
+	      TYPE_TYPELESS_STORAGE (t) = TYPE_TYPELESS_STORAGE (m);
 	    }
 
 	  TYPE_MAIN_VARIANT (t) = m;
Index: gcc/lto/lto.c
===================================================================
--- gcc/lto/lto.c	(revision 246832)
+++ gcc/lto/lto.c	(working copy)
@@ -1161,7 +1161,10 @@  compare_tree_sccs_1 (tree t1, tree t2, t
 	  compare_values (TYPE_FINAL_P);
 	}
       else if (code == ARRAY_TYPE)
-	compare_values (TYPE_NONALIASED_COMPONENT);
+	{
+	  compare_values (TYPE_NONALIASED_COMPONENT);
+	  compare_values (TYPE_TYPELESS_STORAGE);
+	}
       compare_values (TYPE_PACKED);
       compare_values (TYPE_RESTRICT);
       compare_values (TYPE_USER_ALIGN);
Index: gcc/lto-streamer-out.c
===================================================================
--- gcc/lto-streamer-out.c	(revision 246832)
+++ gcc/lto-streamer-out.c	(working copy)
@@ -1142,7 +1142,10 @@  hash_tree (struct streamer_tree_cache_d
 	  hstate.add_flag (TYPE_FINAL_P (t));
 	}
       else if (code == ARRAY_TYPE)
-	hstate.add_flag (TYPE_NONALIASED_COMPONENT (t));
+	{
+	  hstate.add_flag (TYPE_NONALIASED_COMPONENT (t));
+	  hstate.add_flag (TYPE_TYPELESS_STORAGE (t));
+	}
       hstate.commit_flag ();
       hstate.add_int (TYPE_PRECISION (t));
       hstate.add_int (TYPE_ALIGN (t));
Index: gcc/testsuite/g++.dg/torture/pr79671.C
===================================================================
--- gcc/testsuite/g++.dg/torture/pr79671.C	(nonexistent)
+++ gcc/testsuite/g++.dg/torture/pr79671.C	(working copy)
@@ -0,0 +1,25 @@ 
+// { dg-do run }
+
+void *operator new(__SIZE_TYPE__, void *p2) { return p2; }
+struct B { B(int i_) : i(i_) {} int i; };
+struct X
+{
+  unsigned char buf[sizeof (B)];
+};
+
+int __attribute__((noinline)) foo()
+{
+  X x alignas(B), y alignas(B);
+  new (&x) B (0);
+  y = x;
+  B *q = reinterpret_cast <B *>(&y);
+  asm volatile ("" : "=r" (q) : "r" (q));
+  return q->i;
+}
+
+int main()
+{
+  if (foo() != 0)
+    __builtin_abort ();
+  return 0;
+}
Index: gcc/tree-core.h
===================================================================
--- gcc/tree-core.h	(revision 246832)
+++ gcc/tree-core.h	(working copy)
@@ -1511,7 +1511,9 @@  struct GTY(()) tree_type_common {
      so we need to store the value 32 (not 31, as we need the zero
      as well), hence six bits.  */
   unsigned align : 6;
-  unsigned spare : 25;
+  unsigned typeless_storage : 1;
+  unsigned spare : 24;
+
   alias_set_type alias_set;
   tree pointer_to;
   tree reference_to;
Index: gcc/tree-streamer-in.c
===================================================================
--- gcc/tree-streamer-in.c	(revision 246832)
+++ gcc/tree-streamer-in.c	(working copy)
@@ -375,7 +375,10 @@  unpack_ts_type_common_value_fields (stru
       TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
     }
   else if (TREE_CODE (expr) == ARRAY_TYPE)
-    TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
+    {
+      TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
+      TYPE_TYPELESS_STORAGE (expr) = (unsigned) bp_unpack_value (bp, 1);
+    }
   TYPE_PRECISION (expr) = bp_unpack_var_len_unsigned (bp);
   SET_TYPE_ALIGN (expr, bp_unpack_var_len_unsigned (bp));
 #ifdef ACCEL_COMPILER
Index: gcc/tree-streamer-out.c
===================================================================
--- gcc/tree-streamer-out.c	(revision 246832)
+++ gcc/tree-streamer-out.c	(working copy)
@@ -327,7 +327,10 @@  pack_ts_type_common_value_fields (struct
       bp_pack_value (bp, TYPE_FINAL_P (expr), 1);
     }
   else if (TREE_CODE (expr) == ARRAY_TYPE)
-    bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
+    {
+      bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
+      bp_pack_value (bp, TYPE_TYPELESS_STORAGE (expr), 1);
+    }
   bp_pack_var_len_unsigned (bp, TYPE_PRECISION (expr));
   bp_pack_var_len_unsigned (bp, TYPE_ALIGN (expr));
 }
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 246832)
+++ gcc/tree.h	(working copy)
@@ -2035,6 +2035,9 @@  extern machine_mode element_mode (const_
 #define TYPE_NONALIASED_COMPONENT(NODE) \
   (ARRAY_TYPE_CHECK (NODE)->type_common.transparent_aggr_flag)
 
+#define TYPE_TYPELESS_STORAGE(NODE) \
+  (ARRAY_TYPE_CHECK (NODE)->type_common.typeless_storage)
+
 /* Indicated that objects of this type should be laid out in as
    compact a way as possible.  */
 #define TYPE_PACKED(NODE) (TYPE_CHECK (NODE)->base.u.bits.packed_flag)
@@ -4914,7 +4917,7 @@  inline bool
 canonical_type_used_p (const_tree t)
 {
   return !(POINTER_TYPE_P (t)
-	   || TREE_CODE (t) == ARRAY_TYPE
+	   || (TREE_CODE (t) == ARRAY_TYPE && ! TYPE_TYPELESS_STORAGE (t))
 	   || TREE_CODE (t) == VECTOR_TYPE);
 }