Fix ICE in match_asm_constraints_1 (PR inline-asm/84941)

Hi!

The inline-asm here has "1p" constraint and we end up with
(plus (frame) (const_int ...))
as input; even when the matching output is a REG, I'm not really
sure emit_move_insn can handle arbitrary expressions (consider e.g.
"1X" constraint), and calling reg_overlap_mentioned_p on something other
than REG/SUBREG/MEM/constant (and a couple of selected other cases)
will ICE too.  My understanding is that the match_asm_constraints mini-pass
is an optimization, so we can try to handle cases which make sense, but if
there is something too difficult to handle we can just punt and let reload
do its job or error_for_asm if it can't reload it.  Especially when I
believe such input expressions can be there only when the numeric constraint
is mixed with some other one, otherwise it should have been a REG or MEM
or something similar.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-03-19  Jakub Jelinek  <jakub@redhat.com>

	PR inline-asm/84941
	* function.c (match_asm_constraints_1): Don't do the optimization
	if input isn't a REG, SUBREG, MEM or constant.

	* gcc.dg/pr84941.c: New test.


	Jakub

Hi,

On Mon, 19 Mar 2018, Jakub Jelinek wrote:

> > Blaeh.  Note that "1p" is actually invalid:
> > 
> > --------------
> > `0', `1', `2', ... `9'
> >      An operand that matches the specified operand number is allowed.
> >      If a digit is used together with letters within the same
> >      alternative, the digit should come last.
> > --------------
> > 
> > Changing the order to "p1" would disable the transformation as well, 
> > because match_asm_constraints_1() uses strtoul on the constraint start.
> 
> Ah, ok, but
>   asm volatile ("" : "=m,m" (b), "=r,r" (b) : "1,p" (b));
> ICEs the same way, and that should be valid even according to the above
> description.

Yes that's valid and shouldn't ICE.

> > ... this makes sense.  But I think you're too generous in supporting 
> > strange inputs:
> > 
> > >        if (! REG_P (output)
> > >  	  || rtx_equal_p (output, input)
> > >  	  || (GET_MODE (input) != VOIDmode
> > > -	      && GET_MODE (input) != GET_MODE (output)))
> > > +	      && GET_MODE (input) != GET_MODE (output))
> > > +	  || !(REG_P (input) || SUBREG_P (input)
> > > +	       || MEM_P (input) || CONSTANT_P (input)))
> > 
> > I'd only allow REG_P (input) as well, not any of the other forms.
> 
> I'll try to gather some statistics on what kind of inputs appear there
> during bootstrap/regtest and will try to write a few testcases to see
> if just || ! REG_P (output) is sufficient or not.

I think you don't need to try too hard.  The function is designed to 
handle the situation where the matching constraint is a result from an 
input-output constraint, not for improving arbitrary matching constraints.
As such the expected situation is that input and output are lvalues, and 
hence (when their type is anything sensible) gimple registers, and 
therefore pseudos at RTL time.

You could basically reject any constraint that isn't just a single integer 
(i.e. anything with not only digits after the optional '%') and still 
handle the sitatuations for which this function was invented.  I.e. like 
this:

Index: function.c
===================================================================
--- function.c  (revision 254008)
+++ function.c  (working copy)
@@ -6605,7 +6605,7 @@ match_asm_constraints_1 (rtx_insn *insn,
        constraint++;
 
       match = strtoul (constraint, &end, 10);
-      if (end == constraint)
+      if (end == constraint || *end)
        continue;
 
       gcc_assert (match < noutputs);



Ciao,
Michael.

On Mon, Mar 19, 2018 at 08:38:00PM +0000, Michael Matz wrote:
> > Ah, ok, but
> >   asm volatile ("" : "=m,m" (b), "=r,r" (b) : "1,p" (b));
> > ICEs the same way, and that should be valid even according to the above
> > description.
> 
> Yes that's valid and shouldn't ICE.

I will change the testcase in the patch to the above to make it valid.

> > > >        if (! REG_P (output)
> > > >  	  || rtx_equal_p (output, input)
> > > >  	  || (GET_MODE (input) != VOIDmode
> > > > -	      && GET_MODE (input) != GET_MODE (output)))
> > > > +	      && GET_MODE (input) != GET_MODE (output))
> > > > +	  || !(REG_P (input) || SUBREG_P (input)
> > > > +	       || MEM_P (input) || CONSTANT_P (input)))
> > > 
> > > I'd only allow REG_P (input) as well, not any of the other forms.
> > 
> > I'll try to gather some statistics on what kind of inputs appear there
> > during bootstrap/regtest and will try to write a few testcases to see
> > if just || ! REG_P (output) is sufficient or not.
> 
> I think you don't need to try too hard.  The function is designed to 
> handle the situation where the matching constraint is a result from an 
> input-output constraint, not for improving arbitrary matching constraints.
> As such the expected situation is that input and output are lvalues, and 
> hence (when their type is anything sensible) gimple registers, and 
> therefore pseudos at RTL time.

It is very common that input is one of the above cases, during x86_64-linux
and i686-linux bootstraps+regtests I got:
13201x CONST_INT, 1959x MEM, 114x SUBREG, 110x SYMBOL_REF,
2x PLUS (the new testcase only)
and most of those were actually from input-output constraints, like:
  var = 1;
  asm ("" : "+g" (var));
  var2 = &static_var3;
  asm ("" : "+g" (var2));
etc.  I believe the mini-pass does a useful transformation for these that
ought to make it easier for reload to actually handle the matching constraints.

Consider:
long a, b, c, d;
long
f1 (void)
{
  long r = 1;
  long s = 2;
  long t = 3;
  long u = 4;
  asm volatile ("" : "+g" (r), "+g" (s), "+g" (t), "+g" (u)
		: : "ax", "bx", "cx", "dx", "bp", "si", "di",
		    "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15",
		    "xmm0", "xmm1", "xmm2", "xmm3", "xmm4", "xmm5", "xmm6", "xmm7",
		    "xmm8", "xmm9", "xmm10", "xmm11", "xmm12", "xmm13", "xmm14", "xmm15");
  return r + s + t + u;
}

This ought to be reloadable, by spilling r, s, t, u into stack slots and
using NN(%rsp) for those, and the patch with the
	 || !(REG_P (input) || SUBREG_P (input)
              || MEM_P (input) || CONSTANT_P (input)))
rather than just
	 || ! REG_P (input))
attempts to help that by forcing the constants into the output pseudo.
Similarly for &static_var (SYMBOL_REF), loads from some memory (MEM),
or e.g. generic vector casts (SUBREG).  Seems LRA gives up on it anyway,
which to me looks like a LRA bug (it works with "+rm" instead of "+g").

Tried also:
long a, b, c, d;

long
f1 (void)
{
  long r = 1;
  long s = 2;
  long t = 3;
  long u = 4;
  asm volatile ("" : "+g" (r), "+g" (s), "+g" (t), "+g" (u)
		: : "0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12");
  return r + s + t + u;
}
on s390x with -O2 {-mno-lra,-mlra} to test both reload and LRA, LRA ICEs on
it, reload just gives up, but again it ought to be reloadable.

> You could basically reject any constraint that isn't just a single integer 
> (i.e. anything with not only digits after the optional '%') and still 
> handle the sitatuations for which this function was invented.  I.e. like 
> this:
> 
> Index: function.c
> ===================================================================
> --- function.c  (revision 254008)
> +++ function.c  (working copy)
> @@ -6605,7 +6605,7 @@ match_asm_constraints_1 (rtx_insn *insn,
>         constraint++;
>  
>        match = strtoul (constraint, &end, 10);
> -      if (end == constraint)
> +      if (end == constraint || *end)
>         continue;

That wouldn't handle e.g.
  asm volatile ("" : "=m,m" (b), "=r,r" (b) : "1,1" (b));
case.

	Jakub

Hi,

On Tue, 20 Mar 2018, Jakub Jelinek wrote:

> It is very common that input is one of the above cases, during x86_64-linux
> and i686-linux bootstraps+regtests I got:
> 13201x CONST_INT, 1959x MEM, 114x SUBREG, 110x SYMBOL_REF,
> 2x PLUS (the new testcase only)
> and most of those were actually from input-output constraints, like:
>   var = 1;
>   asm ("" : "+g" (var));
>   var2 = &static_var3;
>   asm ("" : "+g" (var2));
> etc.  I believe the mini-pass does a useful transformation for these that
> ought to make it easier for reload to actually handle the matching constraints.

Well, then, so your handling of them makes somewhat sense.

> > -      if (end == constraint)
> > +      if (end == constraint || *end)
> >         continue;
> 
> That wouldn't handle e.g.
>   asm volatile ("" : "=m,m" (b), "=r,r" (b) : "1,1" (b));
> case.

No, it wouldn't, which was my intention.  I'm fine with either way, but 
wouldn't have bothered with these cases.  Human-written asms don't use 
alternatives very often (the asm template can't easily make use of them), 
nor do they use funny matching-or-something-else constraints.  But if you 
want to handle them: more power to you :)


Ciao,
Michael.