| Message ID | b964418b-2d54-3443-03c3-3c114d0d7ec2@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ivopts] Fix fast-math-pr55281.c ICE | expand |
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
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
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
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
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
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
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
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. */