C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Marek Polacek]

Here we are crashing because cxx_fold_indirect_ref got a POINTER_PLUS_EXPR
with offset > signed HOST_WIDE_INT and we tried to convert it to sHWI.

The matching code in fold_indirect_ref_1 uses uHWIs so I've followed suit.
But that code now also uses poly_uint64 and I'm not sure if any of the
constexpr.c code should use it, too.  But this patch fixes the ICE.

Bootstrapped/regtested on x86_64-linux, ok for trunk/7?

2018-01-03  Marek Polacek  <polacek@redhat.com>

	PR c++/83659
	* constexpr.c (cxx_fold_indirect_ref): Use unsigned HOST_WIDE_INT
	when computing offsets.

	* g++.dg/torture/pr83659.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 1aeacd51810..cf7c994b381 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -3109,9 +3109,10 @@ cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base)
 	      && (same_type_ignoring_top_level_qualifiers_p
 		  (type, TREE_TYPE (op00type))))
 	    {
-	      HOST_WIDE_INT offset = tree_to_shwi (op01);
+	      unsigned HOST_WIDE_INT offset = tree_to_uhwi (op01);
 	      tree part_width = TYPE_SIZE (type);
-	      unsigned HOST_WIDE_INT part_widthi = tree_to_shwi (part_width)/BITS_PER_UNIT;
+	      unsigned HOST_WIDE_INT part_widthi
+		= tree_to_uhwi (part_width) / BITS_PER_UNIT;
 	      unsigned HOST_WIDE_INT indexi = offset * BITS_PER_UNIT;
 	      tree index = bitsize_int (indexi);
 
diff --git gcc/testsuite/g++.dg/torture/pr83659.C gcc/testsuite/g++.dg/torture/pr83659.C
index e69de29bb2d..d9f709bb520 100644
--- gcc/testsuite/g++.dg/torture/pr83659.C
+++ gcc/testsuite/g++.dg/torture/pr83659.C
@@ -0,0 +1,11 @@
+// PR c++/83659
+// { dg-do compile }
+
+typedef int V __attribute__ ((__vector_size__ (16)));
+V a;
+
+int
+main ()
+{
+  reinterpret_cast <int *> (&a)[-1] += 1;
+}

	Marek

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Nathan Sidwell]

On 01/03/2018 11:31 AM, Marek Polacek wrote:
> Here we are crashing because cxx_fold_indirect_ref got a POINTER_PLUS_EXPR
> with offset > signed HOST_WIDE_INT and we tried to convert it to sHWI.
> 
> The matching code in fold_indirect_ref_1 uses uHWIs so I've followed suit.
> But that code now also uses poly_uint64 and I'm not sure if any of the
> constexpr.c code should use it, too.  But this patch fixes the ICE.

This doesn't look right to me, but it doesn't help that the test case 
invokes UB.

> +typedef int V __attribute__ ((__vector_size__ (16)));
> +V a;
> +
> +int
> +main ()
> +{
> +  reinterpret_cast <int *> (&a)[-1] += 1;
> +}

one could make this code well formed with (I think)

typedef int V __attribute__ ((__vector_size__ (16)));
V a[2];

int main ()
{
    return reinterpret_cast <int *> (&a[1])[-1];
}

That should access the final element of the a[0] vector.


nathan
-- 
Nathan Sidwell

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Richard Sandiford]

Marek Polacek <polacek@redhat.com> writes:
> Here we are crashing because cxx_fold_indirect_ref got a POINTER_PLUS_EXPR
> with offset > signed HOST_WIDE_INT and we tried to convert it to sHWI.
>
> The matching code in fold_indirect_ref_1 uses uHWIs so I've followed suit.
> But that code now also uses poly_uint64 and I'm not sure if any of the
> constexpr.c code should use it, too.

The frontend isn't supposed to see any poly_ints (yet), so it's OK to
keep on using INTEGER_CST accessors like tree_to_shwi and tree_to_uhwi.

Thanks,
Richard

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Marek Polacek]

On Wed, Jan 03, 2018 at 05:40:32PM +0000, Richard Sandiford wrote:
> Marek Polacek <polacek@redhat.com> writes:
> > Here we are crashing because cxx_fold_indirect_ref got a POINTER_PLUS_EXPR
> > with offset > signed HOST_WIDE_INT and we tried to convert it to sHWI.
> >
> > The matching code in fold_indirect_ref_1 uses uHWIs so I've followed suit.
> > But that code now also uses poly_uint64 and I'm not sure if any of the
> > constexpr.c code should use it, too.
> 
> The frontend isn't supposed to see any poly_ints (yet), so it's OK to
> keep on using INTEGER_CST accessors like tree_to_shwi and tree_to_uhwi.

Thanks, that's good to know.

	Marek

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Marek Polacek]

On Wed, Jan 03, 2018 at 11:45:55AM -0500, Nathan Sidwell wrote:
> On 01/03/2018 11:31 AM, Marek Polacek wrote:
> > Here we are crashing because cxx_fold_indirect_ref got a POINTER_PLUS_EXPR
> > with offset > signed HOST_WIDE_INT and we tried to convert it to sHWI.
> > 
> > The matching code in fold_indirect_ref_1 uses uHWIs so I've followed suit.
> > But that code now also uses poly_uint64 and I'm not sure if any of the
> > constexpr.c code should use it, too.  But this patch fixes the ICE.
> 
> This doesn't look right to me, but it doesn't help that the test case
> invokes UB.

Hmm, like I said, I just followed fold_indirect_ref_1 so I was pretty confident
that this is not a totally wrong thing to do ;).
 
> > +typedef int V __attribute__ ((__vector_size__ (16)));
> > +V a;
> > +
> > +int
> > +main ()
> > +{
> > +  reinterpret_cast <int *> (&a)[-1] += 1;
> > +}
> 
> one could make this code well formed with (I think)
> 
> typedef int V __attribute__ ((__vector_size__ (16)));
> V a[2];
> 
> int main ()
> {
>    return reinterpret_cast <int *> (&a[1])[-1];
> }
> 
> That should access the final element of the a[0] vector.

Unfortunately, this doesn't trigger the ICE.

	Marek

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Richard Biener]

On Wed, Jan 3, 2018 at 5:31 PM, Marek Polacek <polacek@redhat.com> wrote:
> Here we are crashing because cxx_fold_indirect_ref got a POINTER_PLUS_EXPR
> with offset > signed HOST_WIDE_INT and we tried to convert it to sHWI.
>
> The matching code in fold_indirect_ref_1 uses uHWIs so I've followed suit.
> But that code now also uses poly_uint64 and I'm not sure if any of the
> constexpr.c code should use it, too.  But this patch fixes the ICE.

POINTER_PLUS_EXPR offets are to be interpreted as signed (ptrdiff_t)
so using uhwi and then performing an unsigned division is wrong code.
See mem_ref_offset how to deal with this (ugh - poly-ints...).  Basically
you have to force the thing to signed.  Like just use

  HOST_WIDE_INT offset = TREE_INT_CST_LOW (op01);

Richard.

> Bootstrapped/regtested on x86_64-linux, ok for trunk/7?
>
> 2018-01-03  Marek Polacek  <polacek@redhat.com>
>
>         PR c++/83659
>         * constexpr.c (cxx_fold_indirect_ref): Use unsigned HOST_WIDE_INT
>         when computing offsets.
>
>         * g++.dg/torture/pr83659.C: New test.
>
> diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
> index 1aeacd51810..cf7c994b381 100644
> --- gcc/cp/constexpr.c
> +++ gcc/cp/constexpr.c
> @@ -3109,9 +3109,10 @@ cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base)
>               && (same_type_ignoring_top_level_qualifiers_p
>                   (type, TREE_TYPE (op00type))))
>             {
> -             HOST_WIDE_INT offset = tree_to_shwi (op01);
> +             unsigned HOST_WIDE_INT offset = tree_to_uhwi (op01);
>               tree part_width = TYPE_SIZE (type);
> -             unsigned HOST_WIDE_INT part_widthi = tree_to_shwi (part_width)/BITS_PER_UNIT;
> +             unsigned HOST_WIDE_INT part_widthi
> +               = tree_to_uhwi (part_width) / BITS_PER_UNIT;
>               unsigned HOST_WIDE_INT indexi = offset * BITS_PER_UNIT;
>               tree index = bitsize_int (indexi);
>
> diff --git gcc/testsuite/g++.dg/torture/pr83659.C gcc/testsuite/g++.dg/torture/pr83659.C
> index e69de29bb2d..d9f709bb520 100644
> --- gcc/testsuite/g++.dg/torture/pr83659.C
> +++ gcc/testsuite/g++.dg/torture/pr83659.C
> @@ -0,0 +1,11 @@
> +// PR c++/83659
> +// { dg-do compile }
> +
> +typedef int V __attribute__ ((__vector_size__ (16)));
> +V a;
> +
> +int
> +main ()
> +{
> +  reinterpret_cast <int *> (&a)[-1] += 1;
> +}
>
>         Marek

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jakub Jelinek]

On Fri, Jan 05, 2018 at 09:52:36AM +0100, Richard Biener wrote:
> On Wed, Jan 3, 2018 at 5:31 PM, Marek Polacek <polacek@redhat.com> wrote:
> > Here we are crashing because cxx_fold_indirect_ref got a POINTER_PLUS_EXPR
> > with offset > signed HOST_WIDE_INT and we tried to convert it to sHWI.
> >
> > The matching code in fold_indirect_ref_1 uses uHWIs so I've followed suit.
> > But that code now also uses poly_uint64 and I'm not sure if any of the
> > constexpr.c code should use it, too.  But this patch fixes the ICE.
> 
> POINTER_PLUS_EXPR offets are to be interpreted as signed (ptrdiff_t)
> so using uhwi and then performing an unsigned division is wrong code.
> See mem_ref_offset how to deal with this (ugh - poly-ints...).  Basically
> you have to force the thing to signed.  Like just use
> 
>   HOST_WIDE_INT offset = TREE_INT_CST_LOW (op01);

Does it really matter here though?  Any negative offsets there are UB, we
should punt on them rather than try to optimize them.
As we known op01 is unsigned, if we check that it fits into shwi_p, it means
it will be 0 to shwi max and then we can handle it in uhwi too.

 	  /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
 	  if (VECTOR_TYPE_P (op00type)
 	      && (same_type_ignoring_top_level_qualifiers_p
-		 (type, TREE_TYPE (op00type))))
+		 (type, TREE_TYPE (op00type)))
+	      && tree_fits_shwi_p (op01))
 	    {
-	      HOST_WIDE_INT offset = tree_to_shwi (op01);
+	      unsigned HOST_WIDE_INT offset = tree_to_uhwi (op01);
 	      tree part_width = TYPE_SIZE (type);
- 	      unsigned HOST_WIDE_INT part_widthi = tree_to_shwi (part_width)/BITS_PER_UNIT;
+	      unsigned HOST_WIDE_INT part_widthi
+		= tree_to_uhwi (part_width) / BITS_PER_UNIT;
 	      unsigned HOST_WIDE_INT indexi = offset * BITS_PER_UNIT;
 	      tree index = bitsize_int (indexi);
 	  
 	      if (known_lt (offset / part_widthi,
 			    TYPE_VECTOR_SUBPARTS (op00type)))
 		return fold_build3_loc (loc,
 					BIT_FIELD_REF, type, op00,
 					part_width, index);
 		    
 	    }

	Jakub

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Richard Biener]

On Thu, Jan 25, 2018 at 3:45 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 05, 2018 at 09:52:36AM +0100, Richard Biener wrote:
>> On Wed, Jan 3, 2018 at 5:31 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > Here we are crashing because cxx_fold_indirect_ref got a POINTER_PLUS_EXPR
>> > with offset > signed HOST_WIDE_INT and we tried to convert it to sHWI.
>> >
>> > The matching code in fold_indirect_ref_1 uses uHWIs so I've followed suit.
>> > But that code now also uses poly_uint64 and I'm not sure if any of the
>> > constexpr.c code should use it, too.  But this patch fixes the ICE.
>>
>> POINTER_PLUS_EXPR offets are to be interpreted as signed (ptrdiff_t)
>> so using uhwi and then performing an unsigned division is wrong code.
>> See mem_ref_offset how to deal with this (ugh - poly-ints...).  Basically
>> you have to force the thing to signed.  Like just use
>>
>>   HOST_WIDE_INT offset = TREE_INT_CST_LOW (op01);
>
> Does it really matter here though?  Any negative offsets there are UB, we
> should punt on them rather than try to optimize them.
> As we known op01 is unsigned, if we check that it fits into shwi_p, it means
> it will be 0 to shwi max and then we can handle it in uhwi too.

Ah, of course.  Didn't look up enough context to see what this code
does in the end ;)

>           /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
>           if (VECTOR_TYPE_P (op00type)
>               && (same_type_ignoring_top_level_qualifiers_p
> -                (type, TREE_TYPE (op00type))))
> +                (type, TREE_TYPE (op00type)))
> +             && tree_fits_shwi_p (op01))

nevertheless this appearant "mismatch" deserves a comment (checking
shwi and using uhwi).

>             {
> -             HOST_WIDE_INT offset = tree_to_shwi (op01);
> +             unsigned HOST_WIDE_INT offset = tree_to_uhwi (op01);
>               tree part_width = TYPE_SIZE (type);
> -             unsigned HOST_WIDE_INT part_widthi = tree_to_shwi (part_width)/BITS_PER_UNIT;
> +             unsigned HOST_WIDE_INT part_widthi
> +               = tree_to_uhwi (part_width) / BITS_PER_UNIT;
>               unsigned HOST_WIDE_INT indexi = offset * BITS_PER_UNIT;
>               tree index = bitsize_int (indexi);
>
>               if (known_lt (offset / part_widthi,
>                             TYPE_VECTOR_SUBPARTS (op00type)))
>                 return fold_build3_loc (loc,
>                                         BIT_FIELD_REF, type, op00,
>                                         part_width, index);
>
>             }
>
>         Jakub

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jakub Jelinek]

On Fri, Jan 26, 2018 at 02:11:19PM +0100, Richard Biener wrote:
> >> POINTER_PLUS_EXPR offets are to be interpreted as signed (ptrdiff_t)
> >> so using uhwi and then performing an unsigned division is wrong code.
> >> See mem_ref_offset how to deal with this (ugh - poly-ints...).  Basically
> >> you have to force the thing to signed.  Like just use
> >>
> >>   HOST_WIDE_INT offset = TREE_INT_CST_LOW (op01);
> >
> > Does it really matter here though?  Any negative offsets there are UB, we
> > should punt on them rather than try to optimize them.
> > As we known op01 is unsigned, if we check that it fits into shwi_p, it means
> > it will be 0 to shwi max and then we can handle it in uhwi too.
> 
> Ah, of course.  Didn't look up enough context to see what this code
> does in the end ;)
> 
> >           /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
> >           if (VECTOR_TYPE_P (op00type)
> >               && (same_type_ignoring_top_level_qualifiers_p
> > -                (type, TREE_TYPE (op00type))))
> > +                (type, TREE_TYPE (op00type)))
> > +             && tree_fits_shwi_p (op01))
> 
> nevertheless this appearant "mismatch" deserves a comment (checking
> shwi and using uhwi).

So like this?

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-01-26  Marek Polacek  <polacek@redhat.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/83659
	* constexpr.c (cxx_fold_indirect_ref): Use unsigned HOST_WIDE_INT
	when computing offsets.  Verify first that tree_fits_shwi_p (op01).
	Formatting fix.

	* g++.dg/torture/pr83659.C: New test.

--- gcc/cp/constexpr.c.jj	2018-01-24 17:18:42.392392254 +0100
+++ gcc/cp/constexpr.c	2018-01-26 18:54:43.991828743 +0100
@@ -3070,7 +3070,8 @@ cxx_fold_indirect_ref (location_t loc, t
 	{
 	  tree part_width = TYPE_SIZE (type);
 	  tree index = bitsize_int (0);
-	  return fold_build3_loc (loc, BIT_FIELD_REF, type, op, part_width, index);
+	  return fold_build3_loc (loc, BIT_FIELD_REF, type, op, part_width,
+				  index);
 	}
       /* Also handle conversion to an empty base class, which
 	 is represented with a NOP_EXPR.  */
@@ -3109,12 +3110,22 @@ cxx_fold_indirect_ref (location_t loc, t
 
 	  /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
 	  if (VECTOR_TYPE_P (op00type)
-	      && (same_type_ignoring_top_level_qualifiers_p
-		  (type, TREE_TYPE (op00type))))
+	      && same_type_ignoring_top_level_qualifiers_p
+						(type, TREE_TYPE (op00type))
+	      /* POINTER_PLUS_EXPR second operand is sizetype, unsigned,
+		 but we want to treat offsets with MSB set as negative.
+		 For the code below negative offsets are invalid and
+		 TYPE_SIZE of the element is something unsigned, so
+		 check whether op01 fits into HOST_WIDE_INT, which implies
+		 it is from 0 to INTTYPE_MAXIMUM (HOST_WIDE_INT), and
+		 then just use tree_to_uhwi because we want to treat the
+		 value as unsigned.  */
+	      && tree_fits_shwi_p (op01))
 	    {
-	      HOST_WIDE_INT offset = tree_to_shwi (op01);
+	      unsigned HOST_WIDE_INT offset = tree_to_uhwi (op01);
 	      tree part_width = TYPE_SIZE (type);
-	      unsigned HOST_WIDE_INT part_widthi = tree_to_shwi (part_width)/BITS_PER_UNIT;
+	      unsigned HOST_WIDE_INT part_widthi
+		= tree_to_uhwi (part_width) / BITS_PER_UNIT;
 	      unsigned HOST_WIDE_INT indexi = offset * BITS_PER_UNIT;
 	      tree index = bitsize_int (indexi);
 
--- gcc/testsuite/g++.dg/torture/pr83659.C.jj	2018-01-26 18:46:40.077572013 +0100
+++ gcc/testsuite/g++.dg/torture/pr83659.C	2018-01-26 18:56:36.822888606 +0100
@@ -0,0 +1,18 @@
+// PR c++/83659
+// { dg-do compile }
+
+typedef int V __attribute__ ((__vector_size__ (16)));
+V a;
+V b[2];
+
+int
+foo ()
+{
+  return reinterpret_cast <int *> (&a)[-1] += 1;
+}
+
+int
+bar ()
+{
+  return reinterpret_cast <int *> (&a[1])[-1];
+}


	Jakub

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jason Merrill]

On Fri, Jan 26, 2018 at 6:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 26, 2018 at 02:11:19PM +0100, Richard Biener wrote:
>> >> POINTER_PLUS_EXPR offets are to be interpreted as signed (ptrdiff_t)
>> >> so using uhwi and then performing an unsigned division is wrong code.
>> >> See mem_ref_offset how to deal with this (ugh - poly-ints...).  Basically
>> >> you have to force the thing to signed.  Like just use
>> >>
>> >>   HOST_WIDE_INT offset = TREE_INT_CST_LOW (op01);
>> >
>> > Does it really matter here though?  Any negative offsets there are UB, we
>> > should punt on them rather than try to optimize them.
>> > As we known op01 is unsigned, if we check that it fits into shwi_p, it means
>> > it will be 0 to shwi max and then we can handle it in uhwi too.
>>
>> Ah, of course.  Didn't look up enough context to see what this code
>> does in the end ;)
>>
>> >           /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
>> >           if (VECTOR_TYPE_P (op00type)
>> >               && (same_type_ignoring_top_level_qualifiers_p
>> > -                (type, TREE_TYPE (op00type))))
>> > +                (type, TREE_TYPE (op00type)))
>> > +             && tree_fits_shwi_p (op01))
>>
>> nevertheless this appearant "mismatch" deserves a comment (checking
>> shwi and using uhwi).
>
> So like this?
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Why not use the same code as fold_indirect_ref_1 here?

Jason

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Marek Polacek]

On Wed, Feb 07, 2018 at 12:26:04PM -0500, Jason Merrill wrote:
> On Fri, Jan 26, 2018 at 6:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Fri, Jan 26, 2018 at 02:11:19PM +0100, Richard Biener wrote:
> >> >> POINTER_PLUS_EXPR offets are to be interpreted as signed (ptrdiff_t)
> >> >> so using uhwi and then performing an unsigned division is wrong code.
> >> >> See mem_ref_offset how to deal with this (ugh - poly-ints...).  Basically
> >> >> you have to force the thing to signed.  Like just use
> >> >>
> >> >>   HOST_WIDE_INT offset = TREE_INT_CST_LOW (op01);
> >> >
> >> > Does it really matter here though?  Any negative offsets there are UB, we
> >> > should punt on them rather than try to optimize them.
> >> > As we known op01 is unsigned, if we check that it fits into shwi_p, it means
> >> > it will be 0 to shwi max and then we can handle it in uhwi too.
> >>
> >> Ah, of course.  Didn't look up enough context to see what this code
> >> does in the end ;)
> >>
> >> >           /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
> >> >           if (VECTOR_TYPE_P (op00type)
> >> >               && (same_type_ignoring_top_level_qualifiers_p
> >> > -                (type, TREE_TYPE (op00type))))
> >> > +                (type, TREE_TYPE (op00type)))
> >> > +             && tree_fits_shwi_p (op01))
> >>
> >> nevertheless this appearant "mismatch" deserves a comment (checking
> >> shwi and using uhwi).
> >
> > So like this?
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Why not use the same code as fold_indirect_ref_1 here?

That was my first patch, but it was rejected:
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html

	Marek

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jason Merrill]

On Wed, Feb 7, 2018 at 12:54 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Feb 07, 2018 at 12:26:04PM -0500, Jason Merrill wrote:
>> On Fri, Jan 26, 2018 at 6:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Fri, Jan 26, 2018 at 02:11:19PM +0100, Richard Biener wrote:
>> >> >> POINTER_PLUS_EXPR offets are to be interpreted as signed (ptrdiff_t)
>> >> >> so using uhwi and then performing an unsigned division is wrong code.
>> >> >> See mem_ref_offset how to deal with this (ugh - poly-ints...).  Basically
>> >> >> you have to force the thing to signed.  Like just use
>> >> >>
>> >> >>   HOST_WIDE_INT offset = TREE_INT_CST_LOW (op01);
>> >> >
>> >> > Does it really matter here though?  Any negative offsets there are UB, we
>> >> > should punt on them rather than try to optimize them.
>> >> > As we known op01 is unsigned, if we check that it fits into shwi_p, it means
>> >> > it will be 0 to shwi max and then we can handle it in uhwi too.
>> >>
>> >> Ah, of course.  Didn't look up enough context to see what this code
>> >> does in the end ;)
>> >>
>> >> >           /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
>> >> >           if (VECTOR_TYPE_P (op00type)
>> >> >               && (same_type_ignoring_top_level_qualifiers_p
>> >> > -                (type, TREE_TYPE (op00type))))
>> >> > +                (type, TREE_TYPE (op00type)))
>> >> > +             && tree_fits_shwi_p (op01))
>> >>
>> >> nevertheless this appearant "mismatch" deserves a comment (checking
>> >> shwi and using uhwi).
>> >
>> > So like this?
>> >
>> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> Why not use the same code as fold_indirect_ref_1 here?
>
> That was my first patch, but it was rejected:
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html

Then should we update fold_indirect_ref_1 to use the new code?  Is
there a reason for them to stay out of sync?

Jason

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Marek Polacek]

On Wed, Feb 07, 2018 at 01:23:49PM -0500, Jason Merrill wrote:
> On Wed, Feb 7, 2018 at 12:54 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Wed, Feb 07, 2018 at 12:26:04PM -0500, Jason Merrill wrote:
> >> On Fri, Jan 26, 2018 at 6:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> >> > On Fri, Jan 26, 2018 at 02:11:19PM +0100, Richard Biener wrote:
> >> >> >> POINTER_PLUS_EXPR offets are to be interpreted as signed (ptrdiff_t)
> >> >> >> so using uhwi and then performing an unsigned division is wrong code.
> >> >> >> See mem_ref_offset how to deal with this (ugh - poly-ints...).  Basically
> >> >> >> you have to force the thing to signed.  Like just use
> >> >> >>
> >> >> >>   HOST_WIDE_INT offset = TREE_INT_CST_LOW (op01);
> >> >> >
> >> >> > Does it really matter here though?  Any negative offsets there are UB, we
> >> >> > should punt on them rather than try to optimize them.
> >> >> > As we known op01 is unsigned, if we check that it fits into shwi_p, it means
> >> >> > it will be 0 to shwi max and then we can handle it in uhwi too.
> >> >>
> >> >> Ah, of course.  Didn't look up enough context to see what this code
> >> >> does in the end ;)
> >> >>
> >> >> >           /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
> >> >> >           if (VECTOR_TYPE_P (op00type)
> >> >> >               && (same_type_ignoring_top_level_qualifiers_p
> >> >> > -                (type, TREE_TYPE (op00type))))
> >> >> > +                (type, TREE_TYPE (op00type)))
> >> >> > +             && tree_fits_shwi_p (op01))
> >> >>
> >> >> nevertheless this appearant "mismatch" deserves a comment (checking
> >> >> shwi and using uhwi).
> >> >
> >> > So like this?
> >> >
> >> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >>
> >> Why not use the same code as fold_indirect_ref_1 here?
> >
> > That was my first patch, but it was rejected:
> > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html
> 
> Then should we update fold_indirect_ref_1 to use the new code?  Is
> there a reason for them to stay out of sync?

One of the reasons is that middle end uses poly_uint64 type but the front ends
shouldn't use them.  So some of these functions will unfortunately differ.

	Marek

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jason Merrill]

On Wed, Feb 7, 2018 at 2:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Feb 07, 2018 at 08:36:31PM +0100, Marek Polacek wrote:
>> > > That was my first patch, but it was rejected:
>> > > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html
>> >
>> > Then should we update fold_indirect_ref_1 to use the new code?  Is
>> > there a reason for them to stay out of sync?
>>
>> One of the reasons is that middle end uses poly_uint64 type but the front ends
>> shouldn't use them.  So some of these functions will unfortunately differ.
>
> Yeah.  Part of the patch makes the two implementations slightly more
> similar, but I have e.g. no idea how to test for poly_uint64 that fits
> also in poly_int64 and the poly_int* stuff makes the two substantially
> different in any case.

Hmm.  Well, that seems rather unfortunate.  Why shouldn't the front
ends use them?  Can we make an exception for this function because
it's supposed to mirror a middle-end function?
Should we try to push this function back into the middle end?

Jason

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jakub Jelinek]

On Wed, Feb 07, 2018 at 03:23:25PM -0500, Jason Merrill wrote:
> On Wed, Feb 7, 2018 at 2:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Wed, Feb 07, 2018 at 08:36:31PM +0100, Marek Polacek wrote:
> >> > > That was my first patch, but it was rejected:
> >> > > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html
> >> >
> >> > Then should we update fold_indirect_ref_1 to use the new code?  Is
> >> > there a reason for them to stay out of sync?
> >>
> >> One of the reasons is that middle end uses poly_uint64 type but the front ends
> >> shouldn't use them.  So some of these functions will unfortunately differ.
> >
> > Yeah.  Part of the patch makes the two implementations slightly more
> > similar, but I have e.g. no idea how to test for poly_uint64 that fits
> > also in poly_int64 and the poly_int* stuff makes the two substantially
> > different in any case.
> 
> Hmm.  Well, that seems rather unfortunate.  Why shouldn't the front
> ends use them?  Can we make an exception for this function because
> it's supposed to mirror a middle-end function?
> Should we try to push this function back into the middle end?

The function comment seems to explain the reasons:
/* A less strict version of fold_indirect_ref_1, which requires cv-quals to
   match.  We want to be less strict for simple *& folding; if we have a
   non-const temporary that we access through a const pointer, that should
   work.  We handle this here rather than change fold_indirect_ref_1
   because we're dealing with things like ADDR_EXPR of INTEGER_CST which
   don't really make sense outside of constant expression evaluation.  Also
   we want to allow folding to COMPONENT_REF, which could cause trouble
   with TBAA in fold_indirect_ref_1.
   
   Try to keep this function synced with fold_indirect_ref_1.  */

E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
instead of == type comparisons, the COMPONENT_REF stuff, ...

For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
some point, but I could be wrong; certainly it hasn't been done yet and
generally, poly*int seems to be a nightmare to deal with.

	Jakub

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jason Merrill]

On Wed, Feb 7, 2018 at 3:32 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Feb 07, 2018 at 03:23:25PM -0500, Jason Merrill wrote:
>> On Wed, Feb 7, 2018 at 2:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Wed, Feb 07, 2018 at 08:36:31PM +0100, Marek Polacek wrote:
>> >> > > That was my first patch, but it was rejected:
>> >> > > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html
>> >> >
>> >> > Then should we update fold_indirect_ref_1 to use the new code?  Is
>> >> > there a reason for them to stay out of sync?
>> >>
>> >> One of the reasons is that middle end uses poly_uint64 type but the front ends
>> >> shouldn't use them.  So some of these functions will unfortunately differ.
>> >
>> > Yeah.  Part of the patch makes the two implementations slightly more
>> > similar, but I have e.g. no idea how to test for poly_uint64 that fits
>> > also in poly_int64 and the poly_int* stuff makes the two substantially
>> > different in any case.
>>
>> Hmm.  Well, that seems rather unfortunate.  Why shouldn't the front
>> ends use them?  Can we make an exception for this function because
>> it's supposed to mirror a middle-end function?
>> Should we try to push this function back into the middle end?
>
> The function comment seems to explain the reasons:
> /* A less strict version of fold_indirect_ref_1, which requires cv-quals to
>    match.  We want to be less strict for simple *& folding; if we have a
>    non-const temporary that we access through a const pointer, that should
>    work.  We handle this here rather than change fold_indirect_ref_1
>    because we're dealing with things like ADDR_EXPR of INTEGER_CST which
>    don't really make sense outside of constant expression evaluation.  Also
>    we want to allow folding to COMPONENT_REF, which could cause trouble
>    with TBAA in fold_indirect_ref_1.
>
>    Try to keep this function synced with fold_indirect_ref_1.  */
>
> E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
> instead of == type comparisons, the COMPONENT_REF stuff, ...

> For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
> some point, but I could be wrong; certainly it hasn't been done yet and
> generally, poly*int seems to be a nightmare to deal with.

Yes, I understand how we got to this point, but having the functions
diverge because of this guideline seems like a mistake.  And there
seem to be two ways to avoid the divergence: make an exception to the
guideline, or move the function.

Jason

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jason Merrill]

On Wed, Feb 7, 2018 at 4:14 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Feb 07, 2018 at 03:52:39PM -0500, Jason Merrill wrote:
>> > E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
>> > instead of == type comparisons, the COMPONENT_REF stuff, ...
>>
>> > For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
>> > some point, but I could be wrong; certainly it hasn't been done yet and
>> > generally, poly*int seems to be a nightmare to deal with.
>>
>> Yes, I understand how we got to this point, but having the functions
>> diverge because of this guideline seems like a mistake.  And there
>> seem to be two ways to avoid the divergence: make an exception to the
>> guideline, or move the function.
>
> Functionally, I think the following patch should turn fold_indirect_ref_1
> to be equivalent to the patched constexpr.c version (with the known
> documented differences), so if this is the obstackle for the acceptance
> of the patch, I can test this.
>
> Otherwise, I must say I have no idea how to share the code,
> same_type_ignoring_qualifiers is only a C++ FE function, so the middle-end
> can't use it even conditionally, and similarly with the TBAA issues.

Again, can we make an exception and use poly_int in this function
because it's mirroring a middle-end function?

Jason

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jakub Jelinek]

On Wed, Feb 07, 2018 at 08:36:31PM +0100, Marek Polacek wrote:
> > > That was my first patch, but it was rejected:
> > > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html
> > 
> > Then should we update fold_indirect_ref_1 to use the new code?  Is
> > there a reason for them to stay out of sync?
> 
> One of the reasons is that middle end uses poly_uint64 type but the front ends
> shouldn't use them.  So some of these functions will unfortunately differ.

Yeah.  Part of the patch makes the two implementations slightly more
similar, but I have e.g. no idea how to test for poly_uint64 that fits
also in poly_int64 and the poly_int* stuff makes the two substantially
different in any case.

	Jakub

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jakub Jelinek]

On Wed, Feb 07, 2018 at 03:52:39PM -0500, Jason Merrill wrote:
> > E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
> > instead of == type comparisons, the COMPONENT_REF stuff, ...
> 
> > For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
> > some point, but I could be wrong; certainly it hasn't been done yet and
> > generally, poly*int seems to be a nightmare to deal with.
> 
> Yes, I understand how we got to this point, but having the functions
> diverge because of this guideline seems like a mistake.  And there
> seem to be two ways to avoid the divergence: make an exception to the
> guideline, or move the function.

Functionally, I think the following patch should turn fold_indirect_ref_1
to be equivalent to the patched constexpr.c version (with the known
documented differences), so if this is the obstackle for the acceptance
of the patch, I can test this.

Otherwise, I must say I have no idea how to share the code,
same_type_ignoring_qualifiers is only a C++ FE function, so the middle-end
can't use it even conditionally, and similarly with the TBAA issues.

--- gcc/fold-const.c.jj	2018-01-26 18:41:34.316410713 +0100
+++ gcc/fold-const.c	2018-02-07 22:10:54.958628078 +0100
@@ -14172,7 +14172,16 @@ fold_indirect_ref_1 (location_t loc, tre
 
 	  /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
 	  if (TREE_CODE (op00type) == VECTOR_TYPE
-	      && type == TREE_TYPE (op00type))
+	      && type == TREE_TYPE (op00type)
+	      /* POINTER_PLUS_EXPR second operand is sizetype, unsigned,
+		 but we want to treat offsets with MSB set as negative.
+		 For the code below negative offsets are invalid and
+		 TYPE_SIZE of the element is something unsigned, so
+		 check whether op01 fits into poly_int64, which implies
+		 it is from 0 to INTTYPE_MAXIMUM (HOST_WIDE_INT), and
+		 then just use poly_uint64 because we want to treat the
+		 value as unsigned.  */
+	      && tree_fits_poly_int64_p (op01))
 	    {
 	      tree part_width = TYPE_SIZE (type);
 	      poly_uint64 max_offset


	Jakub

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> On Wed, Feb 07, 2018 at 03:23:25PM -0500, Jason Merrill wrote:
>> On Wed, Feb 7, 2018 at 2:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Wed, Feb 07, 2018 at 08:36:31PM +0100, Marek Polacek wrote:
>> >> > > That was my first patch, but it was rejected:
>> >> > > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html
>> >> >
>> >> > Then should we update fold_indirect_ref_1 to use the new code?  Is
>> >> > there a reason for them to stay out of sync?
>> >>
>> >> One of the reasons is that middle end uses poly_uint64 type but the
>> >> front ends
>> >> shouldn't use them.  So some of these functions will unfortunately differ.
>> >
>> > Yeah.  Part of the patch makes the two implementations slightly more
>> > similar, but I have e.g. no idea how to test for poly_uint64 that fits
>> > also in poly_int64 and the poly_int* stuff makes the two substantially
>> > different in any case.
>> 
>> Hmm.  Well, that seems rather unfortunate.  Why shouldn't the front
>> ends use them?  Can we make an exception for this function because
>> it's supposed to mirror a middle-end function?
>> Should we try to push this function back into the middle end?
>
> The function comment seems to explain the reasons:
> /* A less strict version of fold_indirect_ref_1, which requires cv-quals to
>    match.  We want to be less strict for simple *& folding; if we have a
>    non-const temporary that we access through a const pointer, that should
>    work.  We handle this here rather than change fold_indirect_ref_1
>    because we're dealing with things like ADDR_EXPR of INTEGER_CST which
>    don't really make sense outside of constant expression evaluation.  Also
>    we want to allow folding to COMPONENT_REF, which could cause trouble
>    with TBAA in fold_indirect_ref_1.
>    
>    Try to keep this function synced with fold_indirect_ref_1.  */
>
> E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
> instead of == type comparisons, the COMPONENT_REF stuff, ...
>
> For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
> some point, but I could be wrong; certainly it hasn't been done yet and
> generally, poly*int seems to be a nightmare to deal with.

There's no problem with FEs using poly_int now if they want to,
or if it happens to be more convenient in a particular context
(which seems to be the case here to avoid divergence).  It's more
that FEs don't need to go out of their way to handle poly_int,
since the FE can't yet introduce any cases in which the poly_ints
would be nonconstant.

In practice FEs do already use poly_int directly (and indirectly
via support routines).

Thanks,
Richard

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Marek Polacek]

On Thu, Feb 08, 2018 at 10:15:45AM +0000, Richard Sandiford wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> > On Wed, Feb 07, 2018 at 03:23:25PM -0500, Jason Merrill wrote:
> >> On Wed, Feb 7, 2018 at 2:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> >> > On Wed, Feb 07, 2018 at 08:36:31PM +0100, Marek Polacek wrote:
> >> >> > > That was my first patch, but it was rejected:
> >> >> > > https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00271.html
> >> >> >
> >> >> > Then should we update fold_indirect_ref_1 to use the new code?  Is
> >> >> > there a reason for them to stay out of sync?
> >> >>
> >> >> One of the reasons is that middle end uses poly_uint64 type but the
> >> >> front ends
> >> >> shouldn't use them.  So some of these functions will unfortunately differ.
> >> >
> >> > Yeah.  Part of the patch makes the two implementations slightly more
> >> > similar, but I have e.g. no idea how to test for poly_uint64 that fits
> >> > also in poly_int64 and the poly_int* stuff makes the two substantially
> >> > different in any case.
> >> 
> >> Hmm.  Well, that seems rather unfortunate.  Why shouldn't the front
> >> ends use them?  Can we make an exception for this function because
> >> it's supposed to mirror a middle-end function?
> >> Should we try to push this function back into the middle end?
> >
> > The function comment seems to explain the reasons:
> > /* A less strict version of fold_indirect_ref_1, which requires cv-quals to
> >    match.  We want to be less strict for simple *& folding; if we have a
> >    non-const temporary that we access through a const pointer, that should
> >    work.  We handle this here rather than change fold_indirect_ref_1
> >    because we're dealing with things like ADDR_EXPR of INTEGER_CST which
> >    don't really make sense outside of constant expression evaluation.  Also
> >    we want to allow folding to COMPONENT_REF, which could cause trouble
> >    with TBAA in fold_indirect_ref_1.
> >    
> >    Try to keep this function synced with fold_indirect_ref_1.  */
> >
> > E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
> > instead of == type comparisons, the COMPONENT_REF stuff, ...
> >
> > For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
> > some point, but I could be wrong; certainly it hasn't been done yet and
> > generally, poly*int seems to be a nightmare to deal with.
> 
> There's no problem with FEs using poly_int now if they want to,
> or if it happens to be more convenient in a particular context
> (which seems to be the case here to avoid divergence).  It's more
> that FEs don't need to go out of their way to handle poly_int,
> since the FE can't yet introduce any cases in which the poly_ints
> would be nonconstant.
> 
> In practice FEs do already use poly_int directly (and indirectly
> via support routines).

In that case, this patch attemps to synchronize cxx_fold_indirect_ref and
fold_indirect_ref_1 somewhat, especially regarding the poly* stuff.

It also changes if-else-if to if, if, but I can drop that change.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-02-08  Marek Polacek  <polacek@redhat.com>

	PR c++/83659
	* constexpr.c (cxx_fold_indirect_ref): Synchronize with
	fold_indirect_ref_1.

	* g++.dg/torture/pr83659.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 93dd8ae049c..6286c7828c6 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -3025,9 +3025,10 @@ cxx_eval_vec_init (const constexpr_ctx *ctx, tree t,
 static tree
 cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base)
 {
-  tree sub, subtype;
+  tree sub = op0;
+  tree subtype;
+  poly_uint64 const_op01;
 
-  sub = op0;
   STRIP_NOPS (sub);
   subtype = TREE_TYPE (sub);
   if (!POINTER_TYPE_P (subtype))
@@ -3106,8 +3107,9 @@ cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base)
 	      return fold_build3 (COMPONENT_REF, type, op, field, NULL_TREE);
 	}
     }
-  else if (TREE_CODE (sub) == POINTER_PLUS_EXPR
-	   && TREE_CODE (TREE_OPERAND (sub, 1)) == INTEGER_CST)
+
+  if (TREE_CODE (sub) == POINTER_PLUS_EXPR
+      && poly_int_tree_p (TREE_OPERAND (sub, 1), &const_op01))
     {
       tree op00 = TREE_OPERAND (sub, 0);
       tree op01 = TREE_OPERAND (sub, 1);
@@ -3124,26 +3126,25 @@ cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base)
 	      && (same_type_ignoring_top_level_qualifiers_p
 		  (type, TREE_TYPE (op00type))))
 	    {
-	      HOST_WIDE_INT offset = tree_to_shwi (op01);
 	      tree part_width = TYPE_SIZE (type);
-	      unsigned HOST_WIDE_INT part_widthi = tree_to_shwi (part_width)/BITS_PER_UNIT;
-	      unsigned HOST_WIDE_INT indexi = offset * BITS_PER_UNIT;
-	      tree index = bitsize_int (indexi);
-
-	      if (known_lt (offset / part_widthi,
-			    TYPE_VECTOR_SUBPARTS (op00type)))
-		return fold_build3_loc (loc,
-					BIT_FIELD_REF, type, op00,
-					part_width, index);
-
+	      poly_uint64 max_offset
+		= (tree_to_uhwi (part_width) / BITS_PER_UNIT
+		   * TYPE_VECTOR_SUBPARTS (op00type));
+	      if (known_lt (const_op01, max_offset))
+		{
+		  tree index = bitsize_int (const_op01 * BITS_PER_UNIT);
+		  return fold_build3_loc (loc,
+					  BIT_FIELD_REF, type, op00,
+					  part_width, index);
+		}
 	    }
 	  /* ((foo*)&complexfoo)[1] => __imag__ complexfoo */
 	  else if (TREE_CODE (op00type) == COMPLEX_TYPE
 		   && (same_type_ignoring_top_level_qualifiers_p
 		       (type, TREE_TYPE (op00type))))
 	    {
-	      tree size = TYPE_SIZE_UNIT (type);
-	      if (tree_int_cst_equal (size, op01))
+	      if (known_eq (wi::to_poly_offset (TYPE_SIZE_UNIT (type)),
+			    const_op01))
 		return fold_build1_loc (loc, IMAGPART_EXPR, type, op00);
 	    }
 	  /* ((foo *)&fooarray)[1] => fooarray[1] */
@@ -3191,10 +3192,11 @@ cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base)
 	    }
 	}
     }
+
   /* *(foo *)fooarrptr => (*fooarrptr)[0] */
-  else if (TREE_CODE (TREE_TYPE (subtype)) == ARRAY_TYPE
-	   && (same_type_ignoring_top_level_qualifiers_p
-	       (type, TREE_TYPE (TREE_TYPE (subtype)))))
+  if (TREE_CODE (TREE_TYPE (subtype)) == ARRAY_TYPE
+      && (same_type_ignoring_top_level_qualifiers_p
+	  (type, TREE_TYPE (TREE_TYPE (subtype)))))
     {
       tree type_domain;
       tree min_val = size_zero_node;
diff --git gcc/testsuite/g++.dg/torture/pr83659.C gcc/testsuite/g++.dg/torture/pr83659.C
index e69de29bb2d..d9f709bb520 100644
--- gcc/testsuite/g++.dg/torture/pr83659.C
+++ gcc/testsuite/g++.dg/torture/pr83659.C
@@ -0,0 +1,18 @@
+// PR c++/83659
+// { dg-do compile }
+
+typedef int V __attribute__ ((__vector_size__ (16)));
+V a;
+V b[2];
+
+int
+foo ()
+{
+  return reinterpret_cast <int *> (&a)[-1] += 1;
+}
+
+int
+bar ()
+{
+  return reinterpret_cast <int *> (&a[1])[-1];
+}

	Marek

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Marek Polacek]

On Thu, Feb 08, 2018 at 12:55:10PM +0100, Jakub Jelinek wrote:
> On Wed, Feb 07, 2018 at 04:21:43PM -0500, Jason Merrill wrote:
> > On Wed, Feb 7, 2018 at 4:14 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > > On Wed, Feb 07, 2018 at 03:52:39PM -0500, Jason Merrill wrote:
> > >> > E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
> > >> > instead of == type comparisons, the COMPONENT_REF stuff, ...
> > >>
> > >> > For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
> > >> > some point, but I could be wrong; certainly it hasn't been done yet and
> > >> > generally, poly*int seems to be a nightmare to deal with.
> > >>
> > >> Yes, I understand how we got to this point, but having the functions
> > >> diverge because of this guideline seems like a mistake.  And there
> > >> seem to be two ways to avoid the divergence: make an exception to the
> > >> guideline, or move the function.
> > >
> > > Functionally, I think the following patch should turn fold_indirect_ref_1
> > > to be equivalent to the patched constexpr.c version (with the known
> > > documented differences), so if this is the obstackle for the acceptance
> > > of the patch, I can test this.
> > >
> > > Otherwise, I must say I have no idea how to share the code,
> > > same_type_ignoring_qualifiers is only a C++ FE function, so the middle-end
> > > can't use it even conditionally, and similarly with the TBAA issues.
> > 
> > Again, can we make an exception and use poly_int in this function
> > because it's mirroring a middle-end function?
> 
> So like this if it passes bootstrap/regtest?  It is kind of bidirectional
> merge of changes between the 2 functions, except for intentional differences
> (e.g. the same_type_ignoring_top_level_qualifiers_p vs. ==, in_gimple_form
> stuff in fold-const.c, the C++ specific empty class etc. handling in
> constexpr.c etc.).

Better than my patch, and also has comments.  Thanks and sorry for duplicated
effort!

	Marek

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jason Merrill]

On Thu, Feb 8, 2018 at 6:55 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Feb 07, 2018 at 04:21:43PM -0500, Jason Merrill wrote:
>> On Wed, Feb 7, 2018 at 4:14 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Wed, Feb 07, 2018 at 03:52:39PM -0500, Jason Merrill wrote:
>> >> > E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
>> >> > instead of == type comparisons, the COMPONENT_REF stuff, ...
>> >>
>> >> > For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
>> >> > some point, but I could be wrong; certainly it hasn't been done yet and
>> >> > generally, poly*int seems to be a nightmare to deal with.
>> >>
>> >> Yes, I understand how we got to this point, but having the functions
>> >> diverge because of this guideline seems like a mistake.  And there
>> >> seem to be two ways to avoid the divergence: make an exception to the
>> >> guideline, or move the function.
>> >
>> > Functionally, I think the following patch should turn fold_indirect_ref_1
>> > to be equivalent to the patched constexpr.c version (with the known
>> > documented differences), so if this is the obstackle for the acceptance
>> > of the patch, I can test this.
>> >
>> > Otherwise, I must say I have no idea how to share the code,
>> > same_type_ignoring_qualifiers is only a C++ FE function, so the middle-end
>> > can't use it even conditionally, and similarly with the TBAA issues.
>>
>> Again, can we make an exception and use poly_int in this function
>> because it's mirroring a middle-end function?
>
> So like this if it passes bootstrap/regtest?  It is kind of bidirectional
> merge of changes between the 2 functions, except for intentional differences
> (e.g. the same_type_ignoring_top_level_qualifiers_p vs. ==, in_gimple_form
> stuff in fold-const.c, the C++ specific empty class etc. handling in
> constexpr.c etc.).

Looks good, thanks.

Jason

Re: C++ PATCH to fix ICE with vector expr folding (PR c++/83659)

[Jakub Jelinek]

On Wed, Feb 07, 2018 at 04:21:43PM -0500, Jason Merrill wrote:
> On Wed, Feb 7, 2018 at 4:14 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Wed, Feb 07, 2018 at 03:52:39PM -0500, Jason Merrill wrote:
> >> > E.g. the constexpr function uses same_type_ignoring_top_level_qualifiers_p
> >> > instead of == type comparisons, the COMPONENT_REF stuff, ...
> >>
> >> > For poly_* stuff, I think Richard S. wants to introduce it into the FEs at
> >> > some point, but I could be wrong; certainly it hasn't been done yet and
> >> > generally, poly*int seems to be a nightmare to deal with.
> >>
> >> Yes, I understand how we got to this point, but having the functions
> >> diverge because of this guideline seems like a mistake.  And there
> >> seem to be two ways to avoid the divergence: make an exception to the
> >> guideline, or move the function.
> >
> > Functionally, I think the following patch should turn fold_indirect_ref_1
> > to be equivalent to the patched constexpr.c version (with the known
> > documented differences), so if this is the obstackle for the acceptance
> > of the patch, I can test this.
> >
> > Otherwise, I must say I have no idea how to share the code,
> > same_type_ignoring_qualifiers is only a C++ FE function, so the middle-end
> > can't use it even conditionally, and similarly with the TBAA issues.
> 
> Again, can we make an exception and use poly_int in this function
> because it's mirroring a middle-end function?

So like this if it passes bootstrap/regtest?  It is kind of bidirectional
merge of changes between the 2 functions, except for intentional differences
(e.g. the same_type_ignoring_top_level_qualifiers_p vs. ==, in_gimple_form
stuff in fold-const.c, the C++ specific empty class etc. handling in
constexpr.c etc.).

2018-01-26  Marek Polacek  <polacek@redhat.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/83659
	* fold-const.c (fold_indirect_ref_1): Use VECTOR_TYPE_P macro.
	Formatting fixes.  Verify first that tree_fits_poly_int64_p (op01).
	Sync some changes from cxx_fold_indirect_ref.

	* constexpr.c (cxx_fold_indirect_ref): Sync some changes from
	fold_indirect_ref_1, including poly_*int64.  Verify first that
	tree_fits_poly_int64_p (op01).  Formatting fixes.

	* g++.dg/torture/pr83659.C: New test.

--- gcc/fold-const.c.jj	2018-01-26 12:43:23.140922419 +0100
+++ gcc/fold-const.c	2018-02-08 12:43:50.654727317 +0100
@@ -14115,6 +14115,7 @@ fold_indirect_ref_1 (location_t loc, tre
     {
       tree op = TREE_OPERAND (sub, 0);
       tree optype = TREE_TYPE (op);
+
       /* *&CONST_DECL -> to the value of the const decl.  */
       if (TREE_CODE (op) == CONST_DECL)
 	return DECL_INITIAL (op);
@@ -14148,12 +14149,13 @@ fold_indirect_ref_1 (location_t loc, tre
 	       && type == TREE_TYPE (optype))
 	return fold_build1_loc (loc, REALPART_EXPR, type, op);
       /* *(foo *)&vectorfoo => BIT_FIELD_REF<vectorfoo,...> */
-      else if (TREE_CODE (optype) == VECTOR_TYPE
+      else if (VECTOR_TYPE_P (optype)
 	       && type == TREE_TYPE (optype))
 	{
 	  tree part_width = TYPE_SIZE (type);
 	  tree index = bitsize_int (0);
-	  return fold_build3_loc (loc, BIT_FIELD_REF, type, op, part_width, index);
+	  return fold_build3_loc (loc, BIT_FIELD_REF, type, op, part_width,
+				  index);
 	}
     }
 
@@ -14171,8 +14173,17 @@ fold_indirect_ref_1 (location_t loc, tre
 	  op00type = TREE_TYPE (op00);
 
 	  /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
-	  if (TREE_CODE (op00type) == VECTOR_TYPE
-	      && type == TREE_TYPE (op00type))
+	  if (VECTOR_TYPE_P (op00type)
+	      && type == TREE_TYPE (op00type)
+	      /* POINTER_PLUS_EXPR second operand is sizetype, unsigned,
+		 but we want to treat offsets with MSB set as negative.
+		 For the code below negative offsets are invalid and
+		 TYPE_SIZE of the element is something unsigned, so
+		 check whether op01 fits into poly_int64, which implies
+		 it is from 0 to INTTYPE_MAXIMUM (HOST_WIDE_INT), and
+		 then just use poly_uint64 because we want to treat the
+		 value as unsigned.  */
+	      && tree_fits_poly_int64_p (op01))
 	    {
 	      tree part_width = TYPE_SIZE (type);
 	      poly_uint64 max_offset
@@ -14199,16 +14210,16 @@ fold_indirect_ref_1 (location_t loc, tre
 		   && type == TREE_TYPE (op00type))
 	    {
 	      tree type_domain = TYPE_DOMAIN (op00type);
-	      tree min = size_zero_node;
+	      tree min_val = size_zero_node;
 	      if (type_domain && TYPE_MIN_VALUE (type_domain))
-		min = TYPE_MIN_VALUE (type_domain);
+		min_val = TYPE_MIN_VALUE (type_domain);
 	      offset_int off = wi::to_offset (op01);
 	      offset_int el_sz = wi::to_offset (TYPE_SIZE_UNIT (type));
 	      offset_int remainder;
 	      off = wi::divmod_trunc (off, el_sz, SIGNED, &remainder);
-	      if (remainder == 0 && TREE_CODE (min) == INTEGER_CST)
+	      if (remainder == 0 && TREE_CODE (min_val) == INTEGER_CST)
 		{
-		  off = off + wi::to_offset (min);
+		  off = off + wi::to_offset (min_val);
 		  op01 = wide_int_to_tree (sizetype, off);
 		  return build4_loc (loc, ARRAY_REF, type, op00, op01,
 				     NULL_TREE, NULL_TREE);
--- gcc/cp/constexpr.c.jj	2018-02-06 13:12:48.072808498 +0100
+++ gcc/cp/constexpr.c	2018-02-08 12:43:59.526722279 +0100
@@ -3025,9 +3025,10 @@ cxx_eval_vec_init (const constexpr_ctx *
 static tree
 cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base)
 {
-  tree sub, subtype;
+  tree sub = op0;
+  tree subtype;
+  poly_uint64 const_op01;
 
-  sub = op0;
   STRIP_NOPS (sub);
   subtype = TREE_TYPE (sub);
   if (!POINTER_TYPE_P (subtype))
@@ -3082,7 +3083,8 @@ cxx_fold_indirect_ref (location_t loc, t
 	{
 	  tree part_width = TYPE_SIZE (type);
 	  tree index = bitsize_int (0);
-	  return fold_build3_loc (loc, BIT_FIELD_REF, type, op, part_width, index);
+	  return fold_build3_loc (loc, BIT_FIELD_REF, type, op, part_width,
+				  index);
 	}
       /* Also handle conversion to an empty base class, which
 	 is represented with a NOP_EXPR.  */
@@ -3107,7 +3109,7 @@ cxx_fold_indirect_ref (location_t loc, t
 	}
     }
   else if (TREE_CODE (sub) == POINTER_PLUS_EXPR
-	   && TREE_CODE (TREE_OPERAND (sub, 1)) == INTEGER_CST)
+	   && poly_int_tree_p (TREE_OPERAND (sub, 1), &const_op01))
     {
       tree op00 = TREE_OPERAND (sub, 0);
       tree op01 = TREE_OPERAND (sub, 1);
@@ -3121,29 +3123,37 @@ cxx_fold_indirect_ref (location_t loc, t
 
 	  /* ((foo*)&vectorfoo)[1] => BIT_FIELD_REF<vectorfoo,...> */
 	  if (VECTOR_TYPE_P (op00type)
-	      && (same_type_ignoring_top_level_qualifiers_p
-		  (type, TREE_TYPE (op00type))))
+	      && same_type_ignoring_top_level_qualifiers_p
+						(type, TREE_TYPE (op00type))
+	      /* POINTER_PLUS_EXPR second operand is sizetype, unsigned,
+		 but we want to treat offsets with MSB set as negative.
+		 For the code below negative offsets are invalid and
+		 TYPE_SIZE of the element is something unsigned, so
+		 check whether op01 fits into poly_int64, which implies
+		 it is from 0 to INTTYPE_MAXIMUM (HOST_WIDE_INT), and
+		 then just use poly_uint64 because we want to treat the
+		 value as unsigned.  */
+	      && tree_fits_poly_int64_p (op01))
 	    {
-	      HOST_WIDE_INT offset = tree_to_shwi (op01);
 	      tree part_width = TYPE_SIZE (type);
-	      unsigned HOST_WIDE_INT part_widthi = tree_to_shwi (part_width)/BITS_PER_UNIT;
-	      unsigned HOST_WIDE_INT indexi = offset * BITS_PER_UNIT;
-	      tree index = bitsize_int (indexi);
-
-	      if (known_lt (offset / part_widthi,
-			    TYPE_VECTOR_SUBPARTS (op00type)))
-		return fold_build3_loc (loc,
-					BIT_FIELD_REF, type, op00,
-					part_width, index);
-
+	      poly_uint64 max_offset
+		= (tree_to_uhwi (part_width) / BITS_PER_UNIT
+		   * TYPE_VECTOR_SUBPARTS (op00type));
+	      if (known_lt (const_op01, max_offset))
+		{
+		  tree index = bitsize_int (const_op01 * BITS_PER_UNIT);
+		  return fold_build3_loc (loc,
+					  BIT_FIELD_REF, type, op00,
+					  part_width, index);
+		}
 	    }
 	  /* ((foo*)&complexfoo)[1] => __imag__ complexfoo */
 	  else if (TREE_CODE (op00type) == COMPLEX_TYPE
 		   && (same_type_ignoring_top_level_qualifiers_p
 		       (type, TREE_TYPE (op00type))))
 	    {
-	      tree size = TYPE_SIZE_UNIT (type);
-	      if (tree_int_cst_equal (size, op01))
+	      if (known_eq (wi::to_poly_offset (TYPE_SIZE_UNIT (type)),
+			    const_op01))
 		return fold_build1_loc (loc, IMAGPART_EXPR, type, op00);
 	    }
 	  /* ((foo *)&fooarray)[1] => fooarray[1] */
@@ -3198,7 +3208,8 @@ cxx_fold_indirect_ref (location_t loc, t
     {
       tree type_domain;
       tree min_val = size_zero_node;
-      tree newsub = cxx_fold_indirect_ref (loc, TREE_TYPE (subtype), sub, NULL);
+      tree newsub
+	= cxx_fold_indirect_ref (loc, TREE_TYPE (subtype), sub, NULL);
       if (newsub)
 	sub = newsub;
       else
--- gcc/testsuite/g++.dg/torture/pr83659.C.jj	2018-02-08 12:29:11.994353867 +0100
+++ gcc/testsuite/g++.dg/torture/pr83659.C	2018-02-08 12:29:11.994353867 +0100
@@ -0,0 +1,18 @@
+// PR c++/83659
+// { dg-do compile }
+
+typedef int V __attribute__ ((__vector_size__ (16)));
+V a;
+V b[2];
+
+int
+foo ()
+{
+  return reinterpret_cast <int *> (&a)[-1] += 1;
+}
+
+int
+bar ()
+{
+  return reinterpret_cast <int *> (&a[1])[-1];
+}


	Jakub