Do not allow non-allocatable registers in scratch_operand

Currently, scratch_operand allows all hard registers, also those that
cannot be allocated and are only generated explicitly by the backend.

This causes problems.  Consider the case where combine combines
instructions A and B, where B clobbers such a non-allocatable hard reg X,
into an instruction C that has a clobber of a match_scratch.  Concretely
for example:

A is  (set (reg:SI 98) (eq:SI ...))
B is  (parallel [(set (reg:SI 99) (minus (const_int 1) (reg:SI 98)))
                 (clobber (reg:SI CA_REGNO))])

so that combine tries an insn C
(parallel [(set (reg:SI 99) (ne:SI ...))
           (clobber (reg:SI CA_REGNO))])

which matches the pattern
(parallel [(set (match_operand:SI 0 "..." "=r") (ne:SI ...))
           (clobber (match_scratch:SI 3 "=r"))])

which is not going to work; it is not a valid insn.  (In my testcase,
reload eventually replaces CA_REGNO with a GPR that is not dead.  Oops.)

Combine shouldn't keep the original clobbers.  But also, scratch_operand
should not allow non-allocatable registers, since that can never work.
This patch does the latter.

Bootstrapped and regression checked on powerpc64-linux, with options
-m32,-m32/-mpowerpc64,-m64,-m64/-mlra.  No regressions, and testcase
fixed (the testcase is forall_4.f90, it requires some backend patches
to fail).

Is this okay for mainline?


Segher


2014-09-15  Segher Boessenkool  <segher@kernel.crashing.org>

	* recog.c (scratch_operand): Do not simply allow all hard registers:
	only allow those that are allocatable.

---
 gcc/recog.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On 09/15/14 18:58, Segher Boessenkool wrote:
> Currently, scratch_operand allows all hard registers, also those that
> cannot be allocated and are only generated explicitly by the backend.
>
> This causes problems.  Consider the case where combine combines
> instructions A and B, where B clobbers such a non-allocatable hard reg X,
> into an instruction C that has a clobber of a match_scratch.  Concretely
> for example:
>
> A is  (set (reg:SI 98) (eq:SI ...))
> B is  (parallel [(set (reg:SI 99) (minus (const_int 1) (reg:SI 98)))
>                   (clobber (reg:SI CA_REGNO))])
>
> so that combine tries an insn C
> (parallel [(set (reg:SI 99) (ne:SI ...))
>             (clobber (reg:SI CA_REGNO))])
>
> which matches the pattern
> (parallel [(set (match_operand:SI 0 "..." "=r") (ne:SI ...))
>             (clobber (match_scratch:SI 3 "=r"))])
>
> which is not going to work; it is not a valid insn.  (In my testcase,
> reload eventually replaces CA_REGNO with a GPR that is not dead.  Oops.)
>
> Combine shouldn't keep the original clobbers.  But also, scratch_operand
> should not allow non-allocatable registers, since that can never work.
> This patch does the latter.
>
> Bootstrapped and regression checked on powerpc64-linux, with options
> -m32,-m32/-mpowerpc64,-m64,-m64/-mlra.  No regressions, and testcase
> fixed (the testcase is forall_4.f90, it requires some backend patches
> to fail).
>
> Is this okay for mainline?
>
>
> Segher
>
>
> 2014-09-15  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	* recog.c (scratch_operand): Do not simply allow all hard registers:
> 	only allow those that are allocatable.
Shouldn't you be testing if the register is fixed rather than its class? 
  Or maybe both?

jeff

On Thu, Sep 18, 2014 at 11:54:46PM -0600, Jeff Law wrote:
> Shouldn't you be testing if the register is fixed rather than its class? 
>  Or maybe both?

register_operand (via general_operand) uses operand_reg_set for this; it is
initialised via the regclass NO_REGS too (and other things).

This would work for us (rs6000) too, or indeed fixed_regs[], or even wider
classes.  I have no idea if it would hurt other targets though, and I have
no desire to test all weirdo targets ;-)

If another regclass reg is assigned to a match_scratch (and then via a
splitter to a match_operand register), reload can fix it up.  It cannot
fix up things properly for regs of NO_REGS class.  This is what this patch
is for: a class NO_REGS reg cannot ever work as a match_scratch.

I could test for a wider class if you want, but I'll need some guidance
what to test for exactly then; existing code isn't exactly consistent.


Segher

On 09/19/14 06:21, Segher Boessenkool wrote:
> On Thu, Sep 18, 2014 at 11:54:46PM -0600, Jeff Law wrote:
>> Shouldn't you be testing if the register is fixed rather than its class?
>>   Or maybe both?
>
> register_operand (via general_operand) uses operand_reg_set for this; it is
> initialised via the regclass NO_REGS too (and other things).
>
> This would work for us (rs6000) too, or indeed fixed_regs[], or even wider
> classes.  I have no idea if it would hurt other targets though, and I have
> no desire to test all weirdo targets ;-)
Can't blame you for that :-)

Thanks for the pointer to operand_reg_set.  Having walked through some 
of that code, I think we're OK just looking at the class like your 
original patch did.

Concerns withdrawn.  Patch approved.

Jeff