Re: RFA: Tighten checking for 'X' constraints

[Richard Sandiford <rdsandiford at googlemail dot com>]

Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Apr 15, 2014 at 09:53:16PM +0100, Richard Sandiford wrote:
>> As Robert pointed out here:
>> 
>>     http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00416.html
>> 
>> we're a bit too eager when folding stuff into an 'X' constraint.
>> The value at expand time is sensible, but after that asm_operand_ok
>> allows arbitrary rtx expressions, including any number of registers
>> as well as MEMs with unchecked addresses.
>> 
>> This is a target-independent problem, as shown by the testcase below.
>> Reload would give bogus "impossible constraint in asm" errors
>> while LRA ICEs.
>> 
>> Tested on x86_64-linux-gnu.  OK to install?
>
> But then what will be "X" good for compared to "gin" or similar?
> X constraint is meant for operands that aren't really needed, trying to
> print is a user error.
>
> I guess the documentation agrees with this:
>
> 'X'
>      Any operand whatsoever is allowed, even if it does not satisfy
>      'general_operand'.  This is normally used in the constraint of a
>      'match_scratch' when certain alternatives will not actually require
>      a scratch register.
>
> So I think we should just error out if somebody tries to print something
> that satisfies X constraint.

That's the internal documentation though, whereas here we're checking
asm uses.  The documentation for asms just says "Any operand whatsoever
is allowed."  It doesn't say anything about it being unprintable.

I just added the printing side for completeness though.  It wasn't the
point of the patch.  Like I say, the point is that LRA ICEs (on x86_64)
because it can't reload the operands.  Reload couldn't reload them either
but raised "impossible constraint in asm" errors instead of internal errors.
(IMO those errors were also invalid though.  If "X" allows "any operand
whatsoever", how can the operands in the testcase be invalid?)

"X" was defined against reload, which always reloaded MEM addresses
to follow the appropriate base and index register classes.  This was
done as a first pass before matching against the constraints:

  /* Examine each operand that is a memory reference or memory address
     and reload parts of the addresses into index registers.
     Also here any references to pseudo regs that didn't get hard regs
     but are equivalent to constants get replaced in the insn itself
     with those constants.  Nobody will ever see them again.

     Finally, set up the preferred classes of each operand.  */

  for (i = 0; i < noperands; i++)
    {
      ...
      else if (code == MEM)
	{
	  address_reloaded[i]
	    = find_reloads_address (GET_MODE (recog_data.operand[i]),
				    recog_data.operand_loc[i],
				    XEXP (recog_data.operand[i], 0),
				    &XEXP (recog_data.operand[i], 0),
				    i, address_type[i], ind_levels, insn);
	  recog_data.operand[i] = *recog_data.operand_loc[i];
	  substed_operand[i] = recog_data.operand[i];

So I don't think it has ever been the case that "X" allowed MEMs
with arbitrary expressions as the address.

IMO the point of "X" (as implied the doc you quoted) is that it allows
(scratch) operands to be kept as (scratch)s in cases where no scratch
register is needed.

Thanks,
Richard