[ivopts] Fix fast-math-pr55281.c ICE

Message ID b964418b-2d54-3443-03c3-3c114d0d7ec2@codesourcery.com
State New
Headers show
Series
  • [ivopts] Fix fast-math-pr55281.c ICE
Related show

Commit Message

Andrew Stubbs Jan. 28, 2020, 4:52 p.m.
This patch fixes an ICE compiling fast-math-pr55281.c for amdgcn.

The problem is that an "iv" is created in which both base and step are 
pointer types, but the base is converted to sizetype without also 
converting the step to a non-pointer type. Later in the compilation this 
results in a PLUS_EXPR with a pointer argument, which is invalid gimple.

The patch fixes the problem by ensuring that the step is converted at 
the same point the base is. This seems like it ought to be correct. If 
the step is not a pointer type then no conversion occurs.

I don't really understand why I only see this issue on amdgcn, but it 
might be because the pointer in question is in a MASK_LOAD which is 
perhaps not that commonly used?

I've tested this on amdgcn, and done a full bootstrap and test on x86_64 
also.

OK to commit?

Thanks

Andrew

Comments

Richard Biener Jan. 29, 2020, 8:24 a.m. | #1
On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:
>

> This patch fixes an ICE compiling fast-math-pr55281.c for amdgcn.

>

> The problem is that an "iv" is created in which both base and step are

> pointer types,


How did you get a POINTER_TYPE step?  That's where the issue lies
I think.

> but the base is converted to sizetype without also

> converting the step to a non-pointer type. Later in the compilation this

> results in a PLUS_EXPR with a pointer argument, which is invalid gimple.

>

> The patch fixes the problem by ensuring that the step is converted at

> the same point the base is. This seems like it ought to be correct. If

> the step is not a pointer type then no conversion occurs.

>

> I don't really understand why I only see this issue on amdgcn, but it

> might be because the pointer in question is in a MASK_LOAD which is

> perhaps not that commonly used?

>

> I've tested this on amdgcn, and done a full bootstrap and test on x86_64

> also.

>

> OK to commit?

>

> Thanks

>

> Andrew
Andrew Stubbs Jan. 30, 2020, 12:53 p.m. | #2
On 29/01/2020 08:24, Richard Biener wrote:
> On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:

>>

>> This patch fixes an ICE compiling fast-math-pr55281.c for amdgcn.

>>

>> The problem is that an "iv" is created in which both base and step are

>> pointer types,

> 

> How did you get a POINTER_TYPE step?  That's where the issue lies

> I think.


It can come from "find_inv_vars_cb":

   set_iv (idata, op, op, build_int_cst (TREE_TYPE (op), 0), true);

whenever "op" has a pointer type.

Similarly for "get_iv":

   set_iv (data, var, var, build_int_cst (type, 0), true);

whenever "var" has a pointer type.

In this particular case, I traced the origin back to the second one, I 
think (but it's somewhat hard to unpick).

I could change one or both of those, but I don't understand enough about 
the consequences of that to be sure it's the right thing to do. I can 
confirm that converting the step at this point does appear to have the 
desired effect in this instance.

At least at the point of writing it to gimple I can determine what is 
definitely malformed.

Andrew
Bin.Cheng Jan. 30, 2020, 1:03 p.m. | #3
On Thu, Jan 30, 2020 at 8:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:
>

> On 29/01/2020 08:24, Richard Biener wrote:

> > On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:

> >>

> >> This patch fixes an ICE compiling fast-math-pr55281.c for amdgcn.

> >>

> >> The problem is that an "iv" is created in which both base and step are

> >> pointer types,

> >

> > How did you get a POINTER_TYPE step?  That's where the issue lies

> > I think.

>

> It can come from "find_inv_vars_cb":

>

>    set_iv (idata, op, op, build_int_cst (TREE_TYPE (op), 0), true);


This is recording invariant with zero step.  It seems we are using
wrong type building the zero-step.  How about detecting that op has
pointer type and using integer type here?

Thanks,
bin
>

> whenever "op" has a pointer type.

>

> Similarly for "get_iv":

>

>    set_iv (data, var, var, build_int_cst (type, 0), true);

>

> whenever "var" has a pointer type.

>

> In this particular case, I traced the origin back to the second one, I

> think (but it's somewhat hard to unpick).

>

> I could change one or both of those, but I don't understand enough about

> the consequences of that to be sure it's the right thing to do. I can

> confirm that converting the step at this point does appear to have the

> desired effect in this instance.

>

> At least at the point of writing it to gimple I can determine what is

> definitely malformed.

>

> Andrew
Richard Biener Jan. 30, 2020, 1:49 p.m. | #4
On Thu, Jan 30, 2020 at 2:04 PM Bin.Cheng <amker.cheng@gmail.com> wrote:
>

> On Thu, Jan 30, 2020 at 8:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:

> >

> > On 29/01/2020 08:24, Richard Biener wrote:

> > > On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:

> > >>

> > >> This patch fixes an ICE compiling fast-math-pr55281.c for amdgcn.

> > >>

> > >> The problem is that an "iv" is created in which both base and step are

> > >> pointer types,

> > >

> > > How did you get a POINTER_TYPE step?  That's where the issue lies

> > > I think.

> >

> > It can come from "find_inv_vars_cb":

> >

> >    set_iv (idata, op, op, build_int_cst (TREE_TYPE (op), 0), true);

>

> This is recording invariant with zero step.  It seems we are using

> wrong type building the zero-step.  How about detecting that op has

> pointer type and using integer type here?


that sounds like a good idea.

> Thanks,

> bin

> >

> > whenever "op" has a pointer type.

> >

> > Similarly for "get_iv":

> >

> >    set_iv (data, var, var, build_int_cst (type, 0), true);

> >

> > whenever "var" has a pointer type.

> >

> > In this particular case, I traced the origin back to the second one, I

> > think (but it's somewhat hard to unpick).

> >

> > I could change one or both of those, but I don't understand enough about

> > the consequences of that to be sure it's the right thing to do. I can

> > confirm that converting the step at this point does appear to have the

> > desired effect in this instance.

> >

> > At least at the point of writing it to gimple I can determine what is

> > definitely malformed.

> >

> > Andrew
Andrew Stubbs Jan. 30, 2020, 2:09 p.m. | #5
On 30/01/2020 13:49, Richard Biener wrote:
> On Thu, Jan 30, 2020 at 2:04 PM Bin.Cheng <amker.cheng@gmail.com> wrote:

>>

>> On Thu, Jan 30, 2020 at 8:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:

>>>

>>> On 29/01/2020 08:24, Richard Biener wrote:

>>>> On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:

>>>>>

>>>>> This patch fixes an ICE compiling fast-math-pr55281.c for amdgcn.

>>>>>

>>>>> The problem is that an "iv" is created in which both base and step are

>>>>> pointer types,

>>>>

>>>> How did you get a POINTER_TYPE step?  That's where the issue lies

>>>> I think.

>>>

>>> It can come from "find_inv_vars_cb":

>>>

>>>     set_iv (idata, op, op, build_int_cst (TREE_TYPE (op), 0), true);

>>

>> This is recording invariant with zero step.  It seems we are using

>> wrong type building the zero-step.  How about detecting that op has

>> pointer type and using integer type here?

> 

> that sounds like a good idea.


How about this?

I've only tested it on the one testcase, so far, but it works for that.

OK to commit (following a full test)?

Andrew
Fix fast-math-pr55281.c ICE v2

2020-01-30  Andrew Stubbs  <ams@codesourcery.com>

	gcc/
	* tree-ssa-loop-ivopts.c (get_iv): Use sizetype for zero-step.
	(find_inv_vars_cb): Likewise.

diff --git a/gcc/tree-ssa-loop-ivopts.c b/gcc/tree-ssa-loop-ivopts.c
index a21f3077e74..1ce6d8b372b 100644
--- a/gcc/tree-ssa-loop-ivopts.c
+++ b/gcc/tree-ssa-loop-ivopts.c
@@ -1246,7 +1246,11 @@ get_iv (struct ivopts_data *data, tree var)
 
       if (!bb
 	  || !flow_bb_inside_loop_p (data->current_loop, bb))
-	set_iv (data, var, var, build_int_cst (type, 0), true);
+	{
+	  if (POINTER_TYPE_P (type))
+	    type = sizetype;
+	  set_iv (data, var, var, build_int_cst (type, 0), true);
+	}
     }
 
   return name_info (data, var)->iv;
@@ -2990,7 +2994,10 @@ find_inv_vars_cb (tree *expr_p, int *ws ATTRIBUTE_UNUSED, void *data)
 
       if (!bb || !flow_bb_inside_loop_p (idata->current_loop, bb))
 	{
-	  set_iv (idata, op, op, build_int_cst (TREE_TYPE (op), 0), true);
+	  tree steptype = TREE_TYPE (op);
+	  if (POINTER_TYPE_P (steptype))
+	    steptype = sizetype;
+	  set_iv (idata, op, op, build_int_cst (steptype, 0), true);
 	  record_invariant (idata, op, false);
 	}
     }
Richard Biener Jan. 31, 2020, 8:09 a.m. | #6
On Thu, Jan 30, 2020 at 3:09 PM Andrew Stubbs <ams@codesourcery.com> wrote:
>

> On 30/01/2020 13:49, Richard Biener wrote:

> > On Thu, Jan 30, 2020 at 2:04 PM Bin.Cheng <amker.cheng@gmail.com> wrote:

> >>

> >> On Thu, Jan 30, 2020 at 8:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:

> >>>

> >>> On 29/01/2020 08:24, Richard Biener wrote:

> >>>> On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs <ams@codesourcery.com> wrote:

> >>>>>

> >>>>> This patch fixes an ICE compiling fast-math-pr55281.c for amdgcn.

> >>>>>

> >>>>> The problem is that an "iv" is created in which both base and step are

> >>>>> pointer types,

> >>>>

> >>>> How did you get a POINTER_TYPE step?  That's where the issue lies

> >>>> I think.

> >>>

> >>> It can come from "find_inv_vars_cb":

> >>>

> >>>     set_iv (idata, op, op, build_int_cst (TREE_TYPE (op), 0), true);

> >>

> >> This is recording invariant with zero step.  It seems we are using

> >> wrong type building the zero-step.  How about detecting that op has

> >> pointer type and using integer type here?

> >

> > that sounds like a good idea.

>

> How about this?

>

> I've only tested it on the one testcase, so far, but it works for that.

>

> OK to commit (following a full test)?


OK.

Richard.

> Andrew
Andrew Stubbs Jan. 31, 2020, 2:33 p.m. | #7
On 31/01/2020 08:09, Richard Biener wrote:
> On Thu, Jan 30, 2020 at 3:09 PM Andrew Stubbs <ams@codesourcery.com> wrote:

>> How about this?

>>

>> I've only tested it on the one testcase, so far, but it works for that.

>>

>> OK to commit (following a full test)?

> 

> OK.


X86_64 bootstrap and test showed no issues. Nor amdgcn build and test.

Committed, thanks.

Andrew

Patch

Fix fast-math-pr55281.c ICE.

2020-01-28  Andrew Stubbs  <ams@codesourcery.com>

	gcc/
	* tree-ssa-loop-ivopts.c (add_iv_candidate_for_use): Convert step to
	basestep similarly to basetype.

diff --git a/gcc/tree-ssa-loop-ivopts.c b/gcc/tree-ssa-loop-ivopts.c
index a21f3077e74..1abeb13bb53 100644
--- a/gcc/tree-ssa-loop-ivopts.c
+++ b/gcc/tree-ssa-loop-ivopts.c
@@ -3472,7 +3472,7 @@  add_iv_candidate_for_use (struct ivopts_data *data, struct iv_use *use)
 {
   poly_uint64 offset;
   tree base;
-  tree basetype;
+  tree basetype, basestep;
   struct iv *iv = use->iv;
 
   add_candidate (data, iv->base, iv->step, false, use);
@@ -3482,9 +3482,14 @@  add_iv_candidate_for_use (struct ivopts_data *data, struct iv_use *use)
 
   /* Record common candidate with initial value zero.  */
   basetype = TREE_TYPE (iv->base);
+  basestep = iv->step;
   if (POINTER_TYPE_P (basetype))
-    basetype = sizetype;
-  record_common_cand (data, build_int_cst (basetype, 0), iv->step, use);
+    {
+      basetype = sizetype;
+      if (POINTER_TYPE_P (TREE_TYPE (iv->step)))
+	basestep = fold_convert (basetype, iv->step);
+    }
+  record_common_cand (data, build_int_cst (basetype, 0), basestep, use);
 
   /* Compare the cost of an address with an unscaled index with the cost of
     an address with a scaled index and add candidate if useful.  */