, PR target/78658: Fix PowerPC ISA 3.0 convert floating point to char and back

PR target 78658 exposed a thinko that I had in extending the conversion of
QImode and HImode to floating point.  If the QImode/HImode value was in a
vector register, the pattern didn't allocate the second pseudo register, but
the code generated a reference to that second register.  The second register
was used to move the value from a GPR to the vector registers.

I also modified the zero extend patterns to use ^ instead of ?* like I did on
the November 21st patches so that the register allocator is more likely to
consider using the vector registers if it is useful to do so.

I have done a bootstrap build and make check on a little endian power8 system
and there were no regressions.  Can I check this into the trunk?

[gcc]
2016-12-06  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/78658
	* config/rs6000/rs6000.md (zero_extendqi<mode>2): Use ^ instead of
	?* constraints for the ISA 3.0 patterns, so the register allocator
	is more likely to allocate QImode/HImode to vector registers for
	conversion to floating point point unless a reload is needed.
	(zero_extendhi<mode>2): Likewise.
	(float<QHI:mode><FP_ISA3:mode>2_internal): Properly deal with the
	first alternative which is converting QImode/HImode to floating
	point and the QImode/HImode value is in a vector register, and
	does not allocate the second temorary.
	(floatuns<QHI:mode><FP_ISA3:mode>2_internal): Likewise.

[gcc/testsuite]
2016-12-06  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/78658
	* gcc.target/powerpc/pr78658.c: New test.

Hi Mike,

On Tue, Dec 06, 2016 at 03:56:11PM -0500, Michael Meissner wrote:
> PR target 78658 exposed a thinko that I had in extending the conversion of
> QImode and HImode to floating point.  If the QImode/HImode value was in a
> vector register, the pattern didn't allocate the second pseudo register, but
> the code generated a reference to that second register.  The second register
> was used to move the value from a GPR to the vector registers.
> 
> I also modified the zero extend patterns to use ^ instead of ?* like I did on
> the November 21st patches so that the register allocator is more likely to
> consider using the vector registers if it is useful to do so.
> 
> I have done a bootstrap build and make check on a little endian power8 system
> and there were no regressions.  Can I check this into the trunk?

Few things, most trivial (sorry)...

> 	* config/rs6000/rs6000.md (zero_extendqi<mode>2): Use ^ instead of
> 	?* constraints for the ISA 3.0 patterns, so the register allocator
> 	is more likely to allocate QImode/HImode to vector registers for
> 	conversion to floating point point unless a reload is needed.

"point point"

> 	(zero_extendhi<mode>2): Likewise.
> 	(float<QHI:mode><FP_ISA3:mode>2_internal): Properly deal with the
> 	first alternative which is converting QImode/HImode to floating
> 	point and the QImode/HImode value is in a vector register, and
> 	does not allocate the second temorary.

"temorary".

> @@ -5413,8 +5413,11 @@ (define_insn_and_split "*float<QHI:mode>
>  
>    if (!MEM_P (input))
>      {
> +      rtx tmp = operands[3];
>        if (altivec_register_operand (input, <QHI:MODE>mode))
>  	emit_insn (gen_extend<QHI:mode>di2 (di, input));
> +      else if (GET_CODE (tmp) == SCRATCH)
> +	emit_insn (gen_extend<QHI:mode>di2 (di, input));
>        else
>  	{
>  	  rtx tmp = operands[3];

Please remove the extra temporary...  It isn't necessary, and now you
have two variables named tmp (in nested scopes).

> @@ -5449,7 +5452,7 @@ (define_expand "floatuns<QHI:mode><FP_IS
>  (define_insn_and_split "*floatuns<QHI:mode><FP_ISA3:mode>2_internal"
>    [(set (match_operand:FP_ISA3 0 "vsx_register_operand" "=<Fv>,<Fv>,<Fv>")
>  	(unsigned_float:FP_ISA3
> -	 (match_operand:QHI 1 "reg_or_indexed_operand" "wJwK,r,Z")))
> +	 (match_operand:QHI 1 "reg_or_indexed_operand" "wK,r,Z")))
>     (clobber (match_scratch:DI 2 "=wK,wi,wJwK"))
>     (clobber (match_scratch:DI 3 "=X,r,X"))]
>    "TARGET_P9_VECTOR && TARGET_DIRECT_MOVE && TARGET_POWERPC64

Here you remove the wJ alternative, is that on purpose?  The changelog
does not mention it.

Okay for trunk with those things fixed.  Thanks,


Segher

On Tue, Dec 06, 2016 at 03:47:18PM -0600, Segher Boessenkool wrote:
> Hi Mike,
> 
> On Tue, Dec 06, 2016 at 03:56:11PM -0500, Michael Meissner wrote:
> > PR target 78658 exposed a thinko that I had in extending the conversion of
> > QImode and HImode to floating point.  If the QImode/HImode value was in a
> > vector register, the pattern didn't allocate the second pseudo register, but
> > the code generated a reference to that second register.  The second register
> > was used to move the value from a GPR to the vector registers.
> > 
> > I also modified the zero extend patterns to use ^ instead of ?* like I did on
> > the November 21st patches so that the register allocator is more likely to
> > consider using the vector registers if it is useful to do so.
> > 
> > I have done a bootstrap build and make check on a little endian power8 system
> > and there were no regressions.  Can I check this into the trunk?
> 
> Few things, most trivial (sorry)...
> 
> > 	* config/rs6000/rs6000.md (zero_extendqi<mode>2): Use ^ instead of
> > 	?* constraints for the ISA 3.0 patterns, so the register allocator
> > 	is more likely to allocate QImode/HImode to vector registers for
> > 	conversion to floating point point unless a reload is needed.
> 
> "point point"
> 
> > 	(zero_extendhi<mode>2): Likewise.
> > 	(float<QHI:mode><FP_ISA3:mode>2_internal): Properly deal with the
> > 	first alternative which is converting QImode/HImode to floating
> > 	point and the QImode/HImode value is in a vector register, and
> > 	does not allocate the second temorary.

Thanks.

> "temorary".
> 
> > @@ -5413,8 +5413,11 @@ (define_insn_and_split "*float<QHI:mode>
> >  
> >    if (!MEM_P (input))
> >      {
> > +      rtx tmp = operands[3];
> >        if (altivec_register_operand (input, <QHI:MODE>mode))
> >  	emit_insn (gen_extend<QHI:mode>di2 (di, input));
> > +      else if (GET_CODE (tmp) == SCRATCH)
> > +	emit_insn (gen_extend<QHI:mode>di2 (di, input));
> >        else
> >  	{
> >  	  rtx tmp = operands[3];

Thanks.

> Please remove the extra temporary...  It isn't necessary, and now you
> have two variables named tmp (in nested scopes).

Thanks.  I was going back and forth between float<QHI:mode>... and
floatuns<QHI:mode> and I didn't notice the second temporary.

> > @@ -5449,7 +5452,7 @@ (define_expand "floatuns<QHI:mode><FP_IS
> >  (define_insn_and_split "*floatuns<QHI:mode><FP_ISA3:mode>2_internal"
> >    [(set (match_operand:FP_ISA3 0 "vsx_register_operand" "=<Fv>,<Fv>,<Fv>")
> >  	(unsigned_float:FP_ISA3
> > -	 (match_operand:QHI 1 "reg_or_indexed_operand" "wJwK,r,Z")))
> > +	 (match_operand:QHI 1 "reg_or_indexed_operand" "wK,r,Z")))
> >     (clobber (match_scratch:DI 2 "=wK,wi,wJwK"))
> >     (clobber (match_scratch:DI 3 "=X,r,X"))]
> >    "TARGET_P9_VECTOR && TARGET_DIRECT_MOVE && TARGET_POWERPC64
> 
> Here you remove the wJ alternative, is that on purpose?  The changelog
> does not mention it.

Whoops.  I forgot to add the ChangeLog entry for that.  I had the initial
version of these changes in a development branch, and I didn't copy over this
specific ChangeLog entry.

This fixes a potential bug.  The extract byte/half-word instruction only
targets the traditional Altivec registers, so it is important that only Altivec
registers are allowed (i.e. wJ would allow traditional FPRs under ISA 3.0 and
wK would allow traditional Altivec registers under ISA 3.0).  I have updated
the ChangeLog file to note this change.

> Okay for trunk with those things fixed.  Thanks,

Committed, subversion id 243320.