[PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[Jakub Jelinek]

Hi!

VIEW_CONVERT_EXPR<some_type>(0), while REFERENCE_CLASS_P, doesn't have any
base address and is really constant.
In get_references_in_stmt any operand which is DECL_P or REFERENCE_CLASS_P
is added to the references vector, but later on when for each references
vector entry it calls create_data_ref (but in other cases too)
get_base_address is called on it and expected to return non-NULL.
Either we handle get_base_address returning NULL in dr_analyze_alias
and disjoint_objects_p (and additionally need to analyze dr_analyze_indices
etc.), or we don't consider a reference to constant as a reference,
which is what the attached patch does.

Regtested on x86_64-linux, ok for trunk?

2007-10-27  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/33856
	* tree-data-ref.c (get_references_in_stmt): Don't add
	REFERENCE_CLASS_P trees to references vector if get_base_address
	returns NULL on them.

	* gcc.c-torture/compile/20071027-1.c: New test.

--- gcc/tree-data-ref.c.jj	2007-09-05 01:29:01.000000000 +0200
+++ gcc/tree-data-ref.c	2007-10-27 14:48:05.000000000 +0200
@@ -3953,7 +3953,7 @@ get_references_in_stmt (tree stmt, VEC (
       op1 = &GIMPLE_STMT_OPERAND (stmt, 1);
 		
       if (DECL_P (*op1)
-	  || REFERENCE_CLASS_P (*op1))
+	  || (REFERENCE_CLASS_P (*op1) && get_base_address (*op1)))
 	{
 	  ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
 	  ref->pos = op1;
@@ -3961,7 +3961,7 @@ get_references_in_stmt (tree stmt, VEC (
 	}
 
       if (DECL_P (*op0)
-	  || REFERENCE_CLASS_P (*op0))
+	  || (REFERENCE_CLASS_P (*op0) && get_base_address (*op0)))
 	{
 	  ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
 	  ref->pos = op0;
@@ -3978,7 +3978,7 @@ get_references_in_stmt (tree stmt, VEC (
 	  op0 = &CALL_EXPR_ARG (call, i);
 
 	  if (DECL_P (*op0)
-	      || REFERENCE_CLASS_P (*op0))
+	      || (REFERENCE_CLASS_P (*op0) && get_base_address (*op0)))
 	    {
 	      ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
 	      ref->pos = op0;
--- gcc/testsuite/gcc.c-torture/compile/20071027-1.c.jj	2007-10-27 14:09:23.000000000 +0200
+++ gcc/testsuite/gcc.c-torture/compile/20071027-1.c	2007-10-27 14:09:18.000000000 +0200
@@ -0,0 +1,25 @@
+/* PR tree-optimization/33856 */
+/* Testcase by Martin Michlmayr <tbm@cyrius.com> */
+
+typedef struct z_key
+{
+  int key;
+  int mask;
+} z_key;
+typedef struct picture_size
+{
+  z_key key;
+} picture_size;
+
+void picture_size_new (picture_size *ps)
+{
+  z_key key;
+  ps->key = key;
+}
+
+void picture_sizes_load_default (picture_size *ps)
+{
+  int i;
+  for (i = 0; i < 5; ++i)
+    picture_size_new (ps);
+}

	Jakub

Re: [PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[Richard Guenther]

On 10/27/07, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> VIEW_CONVERT_EXPR<some_type>(0), while REFERENCE_CLASS_P, doesn't have any
> base address and is really constant.
> In get_references_in_stmt any operand which is DECL_P or REFERENCE_CLASS_P
> is added to the references vector, but later on when for each references
> vector entry it calls create_data_ref (but in other cases too)
> get_base_address is called on it and expected to return non-NULL.
> Either we handle get_base_address returning NULL in dr_analyze_alias
> and disjoint_objects_p (and additionally need to analyze dr_analyze_indices
> etc.), or we don't consider a reference to constant as a reference,
> which is what the attached patch does.
>
> Regtested on x86_64-linux, ok for trunk?

While there might be cases we (currently) do not fold
VIEW_CONVERT_EXPR<>(cst), did you look at why we do not in this
particular case (what's some_type?).  Possibly a separate PR for
not folding this case is appropriate so we can address this during next
stage1.

Does this PR happen because key is uninitialized?

Otherwise I guess this is ok.

Thanks,
Richard.

> 2007-10-27  Jakub Jelinek  <jakub@redhat.com>
>
>         PR tree-optimization/33856
>         * tree-data-ref.c (get_references_in_stmt): Don't add
>         REFERENCE_CLASS_P trees to references vector if get_base_address
>         returns NULL on them.
>
>         * gcc.c-torture/compile/20071027-1.c: New test.

Re: [PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[Jakub Jelinek]

On Sat, Oct 27, 2007 at 06:33:55PM +0200, Richard Guenther wrote:
> While there might be cases we (currently) do not fold
> VIEW_CONVERT_EXPR<>(cst), did you look at why we do not in this
> particular case (what's some_type?).  Possibly a separate PR for

Yes.  some_type in this case is a RECORD_TYPE, and folding VIEW_CONVERT_EXPR
(particularly in native_interpret_expr) handles only integral, floating
and vector types.  Not sure if it is a good idea to change that (or perhaps
just special case 0 there and turn that into a zero element CONSTRUCTOR of
the corresponding type).  Or perhaps SRA could avoid creating
VIEW_CONVERT_EXPR for RECORD_TYPE/UNION_TYPE.

> not folding this case is appropriate so we can address this during next
> stage1.
> 
> Does this PR happen because key is uninitialized?

Yes, that VIEW_CONVERT_EXPR is created during early SRA (and it a struct
of two ints, so on LP64 it is the same size as long)

	Jakub

Re: [PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[Richard Guenther]

On 10/27/07, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sat, Oct 27, 2007 at 06:33:55PM +0200, Richard Guenther wrote:
> > While there might be cases we (currently) do not fold
> > VIEW_CONVERT_EXPR<>(cst), did you look at why we do not in this
> > particular case (what's some_type?).  Possibly a separate PR for
>
> Yes.  some_type in this case is a RECORD_TYPE, and folding VIEW_CONVERT_EXPR
> (particularly in native_interpret_expr) handles only integral, floating
> and vector types.  Not sure if it is a good idea to change that (or perhaps
> just special case 0 there and turn that into a zero element CONSTRUCTOR of
> the corresponding type).  Or perhaps SRA could avoid creating
> VIEW_CONVERT_EXPR for RECORD_TYPE/UNION_TYPE.
>
> > not folding this case is appropriate so we can address this during next
> > stage1.
> >
> > Does this PR happen because key is uninitialized?
>
> Yes, that VIEW_CONVERT_EXPR is created during early SRA (and it a struct
> of two ints, so on LP64 it is the same size as long)

Interesting.  Why does SRA bother to initialize uninitialized fields?

Richard.

Re: [PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[Jakub Jelinek]

On Sat, Oct 27, 2007 at 07:53:33PM +0200, Richard Guenther wrote:
> > Yes, that VIEW_CONVERT_EXPR is created during early SRA (and it a struct
> > of two ints, so on LP64 it is the same size as long)
> 
> Interesting.  Why does SRA bother to initialize uninitialized fields?

Before esra we have:

picture_size_new (ps)
{
  struct z_key key;

<bb 2>:
  ps_1(D)->key ={v} key;
  return;

}

and whole key var is uninitialized, but assigned.

	Jakub

Re: [PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[Andrew Pinski]

On 10/27/07, Jakub Jelinek <jakub@redhat.com> wrote:
> and whole key var is uninitialized, but assigned.

This was introduced by:
2007-10-01  Alexandre Oliva  <aoliva@redhat.com>

        PR middle-end/22156

I don't see a reason for using 0 for unitialized variables and if we
are going to use that for structs, then an empty CONSTRUCTOR should be
used instead.

Thanks,
Andrew Pinski

Re: [PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[Richard Guenther]

On 10/27/07, Andrew Pinski <pinskia@gmail.com> wrote:
> On 10/27/07, Jakub Jelinek <jakub@redhat.com> wrote:
> > and whole key var is uninitialized, but assigned.
>
> This was introduced by:
> 2007-10-01  Alexandre Oliva  <aoliva@redhat.com>
>
>         PR middle-end/22156
>
> I don't see a reason for using 0 for unitialized variables and if we
> are going to use that for structs, then an empty CONSTRUCTOR should be
> used instead.

I agree.  I also see lots of comparisons like TYPE_MAIN_VARIANT () !=
TYpE_MAIN_VARIANT, instead most of the cases should use
!types_compatible_p () or !useless_type_conversion_p in case you know
what
is assigned to what.

Richard.

Re: [PATCH] Fix tree-data-ref.c ICE on VIEW_CONVERT_EXPR<type>(0) (PR tree-optimization/33856)

[Sebastian Pop]

On 10/27/07, Richard Guenther <richard.guenther@gmail.com> wrote:
> On 10/27/07, Jakub Jelinek <jakub@redhat.com> wrote:
> > Hi!
> >
> > VIEW_CONVERT_EXPR<some_type>(0), while REFERENCE_CLASS_P, doesn't have any
> > base address and is really constant.
> > In get_references_in_stmt any operand which is DECL_P or REFERENCE_CLASS_P
> > is added to the references vector, but later on when for each references
> > vector entry it calls create_data_ref (but in other cases too)
> > get_base_address is called on it and expected to return non-NULL.
> > Either we handle get_base_address returning NULL in dr_analyze_alias
> > and disjoint_objects_p (and additionally need to analyze dr_analyze_indices
> > etc.), or we don't consider a reference to constant as a reference,
> > which is what the attached patch does.
> >
> > Regtested on x86_64-linux, ok for trunk?
>
> Otherwise I guess this is ok.
>

Yes, the fix is ok.

Thanks,
Sebastian