rs6000: Fix PR67344

On Tue, Aug 25, 2015 at 10:08:54AM -0700, Segher Boessenkool wrote:
> -(define_insn_and_split "*and<mode>3_imm_dot_shifted"
> -  [(set (match_operand:CC 3 "cc_reg_operand" "=x,?y")
> +(define_insn "*and<mode>3_imm_dot_shifted"
> +  [(set (match_operand:CC 3 "cc_reg_operand" "=x")

Is this really the best solution?  The operand predicate allows any
cr, but the constraint only cr0.  In the past we've seen this sort of
thing result in "insn does not satisfy its constraints" errors, and if
the operand is successfully reloaded you'll get slow mcrf insns.  At
-O1 the testcase generates:

        andi. 8,3,0x16
        mcrf 4,0

I started throwing together a patch yesterday, before you claimed the
bug.  With this patch, I see what looks to be better code despite it
being larger:

        li 9,22
        and 9,3,9
        cmpdi 4,9,0

On Wed, Aug 26, 2015 at 08:40:49AM +0930, Alan Modra wrote:
> On Tue, Aug 25, 2015 at 10:08:54AM -0700, Segher Boessenkool wrote:
> > -(define_insn_and_split "*and<mode>3_imm_dot_shifted"
> > -  [(set (match_operand:CC 3 "cc_reg_operand" "=x,?y")
> > +(define_insn "*and<mode>3_imm_dot_shifted"
> > +  [(set (match_operand:CC 3 "cc_reg_operand" "=x")
> 
> Is this really the best solution?  The operand predicate allows any
> cr, but the constraint only cr0.  In the past we've seen this sort of
> thing result in "insn does not satisfy its constraints" errors,

I don't think that can happen here.  It is the same situation as
gpc_reg_operand with a "b" constraint.

> and if
> the operand is successfully reloaded you'll get slow mcrf insns.

What is slow about those?  It's not mfcr :-)

Also note that prior compilers (at least as far back as 4.7) generated
it here, too (at -O2 etc. even).

>  At
> -O1 the testcase generates:
> 
>         andi. 8,3,0x16
>         mcrf 4,0
> 
> I started throwing together a patch yesterday, before you claimed the
> bug.  With this patch, I see what looks to be better code despite it
> being larger:
> 
>         li 9,22
>         and 9,3,9
>         cmpdi 4,9,0

The alternative that I was trying to avoid is

	andi. 8,3,0x16
	cmpdi 4,8,0

which doesn't have such a long dependency chain.

> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 87abd6e..a9eea80 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -3060,17 +3060,18 @@
>      return "#";
>  }
>    "&& reload_completed && cc_reg_not_cr0_operand (operands[3], CCmode)"
> -  [(set (match_dup 0)
> -	(and:GPR (lshiftrt:GPR (match_dup 1)
> -			       (match_dup 4))
> -		 (match_dup 2)))
> +  [(set (match_dup 0) (match_dup 2))

This won't work for 0xa000 etc.

> +   (set (match_dup 0) (and:GPR (match_dup 1) (match_dup 0)))
>     (set (match_dup 3)
>  	(compare:CC (match_dup 0)
>  		    (const_int 0)))]
> -  ""
> +  "
> +{
> +  operands[2] = GEN_INT (UINTVAL (operands[2]) << INTVAL (operands[4]));
> +}"
>    [(set_attr "type" "logical")
>     (set_attr "dot" "yes")
> -   (set_attr "length" "4,8")])
> +   (set_attr "length" "4,12")])


Segher

On Tue, Aug 25, 2015 at 07:09:48PM -0500, Segher Boessenkool wrote:
> On Wed, Aug 26, 2015 at 08:40:49AM +0930, Alan Modra wrote:
> > On Tue, Aug 25, 2015 at 10:08:54AM -0700, Segher Boessenkool wrote:
> > > -(define_insn_and_split "*and<mode>3_imm_dot_shifted"
> > > -  [(set (match_operand:CC 3 "cc_reg_operand" "=x,?y")
> > > +(define_insn "*and<mode>3_imm_dot_shifted"
> > > +  [(set (match_operand:CC 3 "cc_reg_operand" "=x")
> > 
> > Is this really the best solution?  The operand predicate allows any
> > cr, but the constraint only cr0.  In the past we've seen this sort of
> > thing result in "insn does not satisfy its constraints" errors,
> 
> I don't think that can happen here.  It is the same situation as
> gpc_reg_operand with a "b" constraint.

Yeah, I'm not sure that it could happen.

> > and if
> > the operand is successfully reloaded you'll get slow mcrf insns.
> 
> What is slow about those?  It's not mfcr :-)

mcrf has a 3 cycle latency on p8, I believe.

> Also note that prior compilers (at least as far back as 4.7) generated
> it here, too (at -O2 etc. even).
> 
> >  At
> > -O1 the testcase generates:
> > 
> >         andi. 8,3,0x16
> >         mcrf 4,0
> > 
> > I started throwing together a patch yesterday, before you claimed the
> > bug.  With this patch, I see what looks to be better code despite it
> > being larger:
> > 
> >         li 9,22
> >         and 9,3,9
> >         cmpdi 4,9,0
> 
> The alternative that I was trying to avoid is
> 
> 	andi. 8,3,0x16
> 	cmpdi 4,8,0
> 
> which doesn't have such a long dependency chain.

But does destroy cr0.

> > +  [(set (match_dup 0) (match_dup 2))
> 
> This won't work for 0xa000 etc.

Oops, yes, you're right.