Re: [RFA:doc] fix PR35492, reload replacing pseudos with constants without checking the pattern condition

[Ian Lance Taylor <iant at google dot com>]

Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:

>> From: Ian Lance Taylor <iant@google.com>
>> Date: Mon, 14 Jul 2008 15:04:58 -0700
>
>> Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
>> > @@ -163,7 +163,9 @@ individual insn, and only after the insn
>> >  recognition template.  The insn's operands may be found in the vector
>> >  @code{operands}.  For an insn where the condition has once matched, it
>> >  can't be used to control register allocation, for example by excluding
>> > -certain hard registers or hard register combinations.
>> > +certain hard registers or hard register combinations.  Also, the
>> > +condition must not be false for a set of operands where the constraints
>> > +of the pattern match.
>> 
>> This does not seem right to me.  It seems to make the condition nearly
>> meaningless.  Certainly the condition is permitted to say that an
>> instruction is only available on certain machines, regardless of
>> whether the constraints match.
>
> Not what I meant!  The sentence is to apply only to expressions
> that inspect operands; the usual TARGET_X test never does.  How
> should I express it?  I tried "the condition, where it checks
> operands, must not be more restrictive than the constraints",
> but I didn't think that'd read very well, especially to
> nonnative readers.  I also certainly don't mean to exclude
> inspection of operands altogether for non-standard-named
> patterns; it just must not refuse what the constraints allow.

It seems to me that that would mean that there is no way to reject
combinations of operands where each operand is acceptable by itself.
I'm not sure that is OK.

For example, the MIPS movdi_32bit insn has this predicate:

  "!TARGET_64BIT && !TARGET_MIPS16
   && (register_operand (operands[0], DImode)
       || reg_or_0_operand (operands[1], DImode))"

I'm not sure we want to say that that is invalid.


>> I may well be missing something here, but this doens't seem right as
>> is.
>
> If you look at the PR, I have this insn:
>
> (insn 12 11 13 3 /home/hp/combm/combined/gcc/testsuite/gcc.c-torture/compile/pr35492.c:18 (set (cc0)
>         (zero_extract:SI (reg:SI 40)
>             (const_int 1 [0x1])
>             (reg:SI 9 r9 [orig:29 <variable>.skc_state ] [29]))) 16 {*btst} (expr_list:REG_DEAD (reg:SI 9 r9 [orig:29 <variable>.skc_state ] [29])
>         (nil)))
>
> In find_reloads, reg 40 is forcibly replaced with a constant
> (one could say that reg 40 didn't get a hard register, but the
> main point in find_reloads is that it's known equal to a
> constant):
>
> (insn 12 11 13 3 /home/hp/combm/combined/gcc/testsuite/gcc.c-torture/compile/pr35492.c:18 (set (cc0)
>         (zero_extract:SI (const_int -13 [0xfffffffffffffff3])
>             (const_int 1 [0x1])
>             (reg:SI 9 r9 [orig:29 <variable>.skc_state ] [29]))) 16 {*btst} (expr_list:REG_DEAD (reg:SI 9 r9 [orig:29 <variable>.skc_state ] [29])
>         (nil)))
>
> (not actually replaced in the insn at this time, just in
> recog_data.operand)
>
> The condition did not allow this replacement, but the predicates
> and the constraints do, but that's to match an insn like this:
>  (zero_extract:SI (const_int 64) (const_int 1) (reg:SI 9 r9))
> where operand 0 is a power of 2, arguably a case of being
> too-smart, see the nearby comment.
>
> The insn passes find_reloads predicates and constraints tests
> with flying colors, because only the condition said that the
> insn with -13 was invalid; no reloads are recorded.  For
> all-but-the-last find_reloads calls (where replace = 0), it
> *does* return 1, see c:a line 4116, because substed_operand[0]
> != *recog_data.operand_loc[0], but that just tells that some
> operand will be changed; it doesn't mark any reloads.

Why not just have the operand predicate and the operand constraint
require an exact power of two when operand 0 is an integer?

It seems to me that your proposal will declare existing MD files to be
invalid, and I don't see what we gain in practice.

Ian