Message ID | 20171122091718.GH14653@tucnak |
---|---|
State | New |
Headers | show |
Series | Fix mult expansion ICE (PR middle-end/82875) | expand |
On Wed, 22 Nov 2017, Jakub Jelinek wrote: > Hi! > > On these two testcases, we end up expanding MULT_EXPR where both arguments > end up being VOIDmode. For smul_optab that isn't a problem, we have > the mode next to it, but in some cases we want to use {u,s}mul_widen_optab > which is a conversion optab which needs two modes expand_binop is called > just with one mode, the result mode, so the other mode is guessed from > the operands and if both are VOIDmode, then a fallback is chosen to use > return mode. The new find_widening* changes ICE on that though, previously > we'd just do something. > > In any case, I think we need to make sure this doesn't happen while we > still know both modes for the {u,s}mul_widen_optab. Bootstrapped/regtested > on x86_64-linux and i686-linux, ok for trunk? > > Perhaps additionally we could have somewhere a case which for both arguments > constant (unlikely case, as the gimple optimizers should usually optimize > that out) and selected optabs for which we know the corresponding RTL code > we could use simplify_const_binary_operation and see if it optimizes into a > constant and just return that. Though, these functions are large and it > is still possible a constant could be uncovered later, so I think we want > this patch even if we do something like that. How much churn would it be to pass down a mode alongside the operands in expand_binop? Can't find it right now but didn't we introduce some rtx_with_mode pair-like stuff somewhen? Anyway, the patch looks ok to me but generally we of course want to pass down modes from where we still know them. Richard. > 2017-11-22 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/82875 > * optabs.c (expand_doubleword_mult, expand_binop): Before calling > expand_binop with *mul_widen_optab, make sure at least one of the > operands doesn't have VOIDmode. > > * gcc.dg/pr82875.c: New test. > * gcc.c-torture/compile/pr82875.c: New test. > > --- gcc/optabs.c.jj 2017-11-09 20:23:51.000000000 +0100 > +++ gcc/optabs.c 2017-11-21 17:40:13.318673366 +0100 > @@ -861,6 +861,11 @@ expand_doubleword_mult (machine_mode mod > if (target && !REG_P (target)) > target = NULL_RTX; > > + /* *_widen_optab needs to determine operand mode, make sure at least > + one operand has non-VOID mode. */ > + if (GET_MODE (op0_low) == VOIDmode && GET_MODE (op1_low) == VOIDmode) > + op0_low = force_reg (word_mode, op0_low); > + > if (umulp) > product = expand_binop (mode, umul_widen_optab, op0_low, op1_low, > target, 1, OPTAB_DIRECT); > @@ -1199,6 +1204,10 @@ expand_binop (machine_mode mode, optab b > : smul_widen_optab), > wider_mode, mode) != CODE_FOR_nothing)) > { > + /* *_widen_optab needs to determine operand mode, make sure at least > + one operand has non-VOID mode. */ > + if (GET_MODE (op0) == VOIDmode && GET_MODE (op1) == VOIDmode) > + op0 = force_reg (mode, op0); > temp = expand_binop (wider_mode, > unsignedp ? umul_widen_optab : smul_widen_optab, > op0, op1, NULL_RTX, unsignedp, OPTAB_DIRECT); > --- gcc/testsuite/gcc.dg/pr82875.c.jj 2017-11-21 17:50:16.022274628 +0100 > +++ gcc/testsuite/gcc.dg/pr82875.c 2017-11-21 17:49:46.000000000 +0100 > @@ -0,0 +1,11 @@ > +/* PR middle-end/82875 */ > +/* { dg-do compile } */ > +/* { dg-options "-ftree-ter" } */ > + > +const int a = 100; > + > +void > +foo (void) > +{ > + int c[a]; > +} > --- gcc/testsuite/gcc.c-torture/compile/pr82875.c.jj 2017-11-21 17:48:46.409374708 +0100 > +++ gcc/testsuite/gcc.c-torture/compile/pr82875.c 2017-11-21 17:48:25.000000000 +0100 > @@ -0,0 +1,24 @@ > +/* PR middle-end/82875 */ > + > +signed char a; > +unsigned b; > +long c, d; > +long long e; > + > +void > +foo (void) > +{ > + short f = a = 6; > + while (0) > + while (a <= 7) > + { > + for (;;) > + ; > + lab: > + while (c <= 73) > + ; > + e = 20; > + d ? (a %= c) * (e *= a ? f : b) : 0; > + } > + goto lab; > +} > > Jakub > >
Really sorry for missing this PR -- don't know that happened :-( Jakub Jelinek <jakub@redhat.com> writes: > On these two testcases, we end up expanding MULT_EXPR where both arguments > end up being VOIDmode. For smul_optab that isn't a problem, we have > the mode next to it, but in some cases we want to use {u,s}mul_widen_optab > which is a conversion optab which needs two modes expand_binop is called > just with one mode, the result mode, so the other mode is guessed from > the operands and if both are VOIDmode, then a fallback is chosen to use > return mode. The new find_widening* changes ICE on that though, previously > we'd just do something. What do you think about passing the modes of the operands down to expand_binop too, a bit like simplify_unary_operation? We could have an overloaded wrapper with the current interface to avoid updating every caller. That at least would cut down on some of the guessing that the function currently does. I can have a go at that if it sounds OK, but the posted patch LGTM too as an alternative. > In any case, I think we need to make sure this doesn't happen while we > still know both modes for the {u,s}mul_widen_optab. Bootstrapped/regtested > on x86_64-linux and i686-linux, ok for trunk? > > Perhaps additionally we could have somewhere a case which for both arguments > constant (unlikely case, as the gimple optimizers should usually optimize > that out) and selected optabs for which we know the corresponding RTL code > we could use simplify_const_binary_operation and see if it optimizes into a > constant and just return that. Though, these functions are large and it > is still possible a constant could be uncovered later, so I think we want > this patch even if we do something like that. (FWIW, I agree we need this either way, although folding sounds good too.) Thanks, Richard
On Wed, Nov 22, 2017 at 10:41:19AM +0100, Richard Biener wrote: > How much churn would it be to pass down a mode alongside the operands > in expand_binop? Can't find it right now but didn't we introduce > some rtx_with_mode pair-like stuff somewhen? We have rtx_mode_t for that. But there are 240+ calls to expand_binop, and even if we add an overload that will transform it, unless we forcefully inline it wouldn't that slow down all the spots a little bit? The thing is, for the vast majority of binary ops we don't need the operand modes, it is mainly comparisons, second arg of shifts/rotates and this widening case. Jakub
On Wed, 22 Nov 2017, Jakub Jelinek wrote: > On Wed, Nov 22, 2017 at 10:41:19AM +0100, Richard Biener wrote: > > How much churn would it be to pass down a mode alongside the operands > > in expand_binop? Can't find it right now but didn't we introduce > > some rtx_with_mode pair-like stuff somewhen? > > We have rtx_mode_t for that. But there are 240+ calls to expand_binop, > and even if we add an overload that will transform it, unless we forcefully > inline it wouldn't that slow down all the spots a little bit? > The thing is, for the vast majority of binary ops we don't need the operand > modes, it is mainly comparisons, second arg of shifts/rotates and this > widening case. Ok, so maybe split expand_binop then to the class of cases where we do need the mode and a class where we don't then? We don't have to use rtx_mode_t we can just pass two arguments. Not sure what is more convenient to use. Anyway, this doesn't have to happen in stage3, just as a general note on how I believe we changed things in other places. Richard S. may remember more here. Richard.
Richard Biener <rguenther@suse.de> writes: > On Wed, 22 Nov 2017, Jakub Jelinek wrote: > >> On Wed, Nov 22, 2017 at 10:41:19AM +0100, Richard Biener wrote: >> > How much churn would it be to pass down a mode alongside the operands >> > in expand_binop? Can't find it right now but didn't we introduce >> > some rtx_with_mode pair-like stuff somewhen? >> >> We have rtx_mode_t for that. But there are 240+ calls to expand_binop, >> and even if we add an overload that will transform it, unless we forcefully >> inline it wouldn't that slow down all the spots a little bit? >> The thing is, for the vast majority of binary ops we don't need the operand >> modes, it is mainly comparisons, second arg of shifts/rotates and this >> widening case. > > Ok, so maybe split expand_binop then to the class of cases where we do > need the mode and a class where we don't then? > > We don't have to use rtx_mode_t we can just pass two arguments. Not > sure what is more convenient to use. FWIW, rtx_mode_t was only really added so that we have a single blob for wi:: calls (with the hope that it would eventually be replaced with just the rtx once CONST_INTs have a mode). I think it'd be more consistent to use separate arguments for other cases. Thanks, Richard > Anyway, this doesn't have to happen in stage3, just as a general > note on how I believe we changed things in other places. Richard S. > may remember more here. > > Richard.
On Wed, Nov 22, 2017 at 01:03:06PM +0000, Richard Sandiford wrote: > Richard Biener <rguenther@suse.de> writes: > > On Wed, 22 Nov 2017, Jakub Jelinek wrote: > > > >> On Wed, Nov 22, 2017 at 10:41:19AM +0100, Richard Biener wrote: > >> > How much churn would it be to pass down a mode alongside the operands > >> > in expand_binop? Can't find it right now but didn't we introduce > >> > some rtx_with_mode pair-like stuff somewhen? > >> > >> We have rtx_mode_t for that. But there are 240+ calls to expand_binop, > >> and even if we add an overload that will transform it, unless we forcefully > >> inline it wouldn't that slow down all the spots a little bit? > >> The thing is, for the vast majority of binary ops we don't need the operand > >> modes, it is mainly comparisons, second arg of shifts/rotates and this > >> widening case. > > > > Ok, so maybe split expand_binop then to the class of cases where we do > > need the mode and a class where we don't then? > > > > We don't have to use rtx_mode_t we can just pass two arguments. Not > > sure what is more convenient to use. > > FWIW, rtx_mode_t was only really added so that we have a single blob > for wi:: calls (with the hope that it would eventually be replaced > with just the rtx once CONST_INTs have a mode). I think it'd be > more consistent to use separate arguments for other cases. So perhaps it would be easiest to just add one defaulted to VOIDmode argument to expand_binop, say aux_mode, which would stand for whatever other second mode the optab needs (could be operand mode for comparisons or the widening stuff, or op1 mode for shifts, etc.). If it is VOIDmode, it wouldn't be used. Jakub
--- gcc/optabs.c.jj 2017-11-09 20:23:51.000000000 +0100 +++ gcc/optabs.c 2017-11-21 17:40:13.318673366 +0100 @@ -861,6 +861,11 @@ expand_doubleword_mult (machine_mode mod if (target && !REG_P (target)) target = NULL_RTX; + /* *_widen_optab needs to determine operand mode, make sure at least + one operand has non-VOID mode. */ + if (GET_MODE (op0_low) == VOIDmode && GET_MODE (op1_low) == VOIDmode) + op0_low = force_reg (word_mode, op0_low); + if (umulp) product = expand_binop (mode, umul_widen_optab, op0_low, op1_low, target, 1, OPTAB_DIRECT); @@ -1199,6 +1204,10 @@ expand_binop (machine_mode mode, optab b : smul_widen_optab), wider_mode, mode) != CODE_FOR_nothing)) { + /* *_widen_optab needs to determine operand mode, make sure at least + one operand has non-VOID mode. */ + if (GET_MODE (op0) == VOIDmode && GET_MODE (op1) == VOIDmode) + op0 = force_reg (mode, op0); temp = expand_binop (wider_mode, unsignedp ? umul_widen_optab : smul_widen_optab, op0, op1, NULL_RTX, unsignedp, OPTAB_DIRECT); --- gcc/testsuite/gcc.dg/pr82875.c.jj 2017-11-21 17:50:16.022274628 +0100 +++ gcc/testsuite/gcc.dg/pr82875.c 2017-11-21 17:49:46.000000000 +0100 @@ -0,0 +1,11 @@ +/* PR middle-end/82875 */ +/* { dg-do compile } */ +/* { dg-options "-ftree-ter" } */ + +const int a = 100; + +void +foo (void) +{ + int c[a]; +} --- gcc/testsuite/gcc.c-torture/compile/pr82875.c.jj 2017-11-21 17:48:46.409374708 +0100 +++ gcc/testsuite/gcc.c-torture/compile/pr82875.c 2017-11-21 17:48:25.000000000 +0100 @@ -0,0 +1,24 @@ +/* PR middle-end/82875 */ + +signed char a; +unsigned b; +long c, d; +long long e; + +void +foo (void) +{ + short f = a = 6; + while (0) + while (a <= 7) + { + for (;;) + ; + lab: + while (c <= 73) + ; + e = 20; + d ? (a %= c) * (e *= a ? f : b) : 0; + } + goto lab; +}