Fix mult expansion ICE (PR middle-end/82875)

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.

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.


	Jakub

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