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

[Segher Boessenkool <segher at kernel dot crashing dot org>]

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