Re: [PATCH, ivopts] Fix fast-math-pr55281.c ICE
On 31/01/2020 08:09, Richard Biener wrote: On Thu, Jan 30, 2020 at 3:09 PM Andrew Stubbs 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
Re: [PATCH, ivopts] Fix fast-math-pr55281.c ICE
On Thu, Jan 30, 2020 at 3:09 PM Andrew Stubbs wrote: > > On 30/01/2020 13:49, Richard Biener wrote: > > On Thu, Jan 30, 2020 at 2:04 PM Bin.Cheng wrote: > >> > >> On Thu, Jan 30, 2020 at 8:53 PM Andrew Stubbs > >> wrote: > >>> > >>> On 29/01/2020 08:24, Richard Biener wrote: > On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs > 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
Re: [PATCH, ivopts] Fix fast-math-pr55281.c ICE
On 30/01/2020 13:49, Richard Biener wrote:
On Thu, Jan 30, 2020 at 2:04 PM Bin.Cheng wrote:
On Thu, Jan 30, 2020 at 8:53 PM Andrew Stubbs wrote:
On 29/01/2020 08:24, Richard Biener wrote:
On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs 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
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);
}
}
Re: [PATCH, ivopts] Fix fast-math-pr55281.c ICE
On Thu, Jan 30, 2020 at 2:04 PM Bin.Cheng wrote: > > On Thu, Jan 30, 2020 at 8:53 PM Andrew Stubbs wrote: > > > > On 29/01/2020 08:24, Richard Biener wrote: > > > On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs > > > 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
Re: [PATCH, ivopts] Fix fast-math-pr55281.c ICE
On Thu, Jan 30, 2020 at 8:53 PM Andrew Stubbs wrote: > > On 29/01/2020 08:24, Richard Biener wrote: > > On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs 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
Re: [PATCH, ivopts] Fix fast-math-pr55281.c ICE
On 29/01/2020 08:24, Richard Biener wrote: On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs 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
Re: [PATCH, ivopts] Fix fast-math-pr55281.c ICE
On Tue, Jan 28, 2020 at 5:53 PM Andrew Stubbs 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
[PATCH, ivopts] Fix fast-math-pr55281.c ICE
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
Fix fast-math-pr55281.c ICE.
2020-01-28 Andrew Stubbs
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. */
