Re: [PATCH 3/3] Add IEEE 128-bit fp conditional move on PowerPC.

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Tue, Jun 08, 2021 at 08:24:47PM -0400, Michael Meissner wrote:
> In this patch, I simplified things compared to previous patches.  Instead of
> allowing any four of the modes to be used for the conditional move comparison
> and the move itself could use different modes, I restricted the conditional
> move to just the same mode.  I.e. you can do:
> 
>         _Float128 a, b, c, d, e, r;
> 
>         r = (a == b) ? c : d;
> 
> But you can't do:
> 
>         _Float128 c, d, r;
>         double a, b;
> 
>         r = (a == b) ? c : d;
> 
> or:
> 
>         _Float128 a, b;
>         double c, d, r;
> 
>         r = (a == b) ? c : d;

I suspect you mean you *can* do it, but it generates sub-optimal code
for it, promoting everything to QP float?

> This eliminates a lot of the complexity of the code, because you don't have to
> worry about the sizes being different, and the IEEE 128-bit types being
> restricted to Altivec registers, while the SF/DF modes can use any VSX
> register.

It doesn't remove any complexity.  Instead, you postpone having to work
on it to a later date.  Pootentially making it harder for the person who
will eventually have to do it to do a good job :-(

(Or this code is not worth having in the first place, in which case, why
do we want to have it?)

Of course this is the more common case, but it is not so overwhelmingly
more common that we can ignore the rest.

> Compared to the May 18th patches, this patch replaces the complicated test that
> was complained about.

That was explained to you to not be a good idea.  Yes.

> @@ -15783,6 +15784,35 @@ rs6000_maybe_emit_fp_cmove (rtx dest, rtx op, rtx true_cond, rtx false_cond)
>    if (!can_create_pseudo_p ())
>      return 0;
>  
> +  /* We allow the comparison to be either SFmode/DFmode and the true/false
> +     condition to be either SFmode/DFmode.  I.e. we allow:
> +
> +	float a, b;
> +	double c, d, r;
> +
> +	r = (a == b) ? c : d;
> +
> +    and:
> +
> +	double a, b;
> +	float c, d, r;
> +
> +	r = (a == b) ? c : d;
> +
> +    but we don't allow intermixing the IEEE 128-bit floating point types with
> +    the 32/64-bit scalar types.
> +
> +    It gets too messy where SFmode/DFmode can use any register and TFmode/KFmode
> +    can only use Altivec registers.  In addtion, we would need to do a XXPERMDI

(spelling: "addition", "an").

Lose the "gets too messy" comment, people might believe it!

> +  if (compare_mode == result_mode
> +      || (compare_mode == SFmode && result_mode == DFmode)
> +      || (compare_mode == DFmode && result_mode == SFmode))
> +    ;
> +  else
> +    return false;

Use a logical inversion, instead, as said before:

  if (!(compare_mode == result_mode
	|| (compare_mode == SFmode && result_mode == DFmode)
	|| (compare_mode == DFmode && result_mode == SFmode)))
    return false;

> +;; Support for ISA 3.1 IEEE 128-bit conditional move.  The mode used in the
> +;; comparison must be the same as used in the conditional move.

s/conditional //

Okay for trunk with those fixes.  Also okay for 11 after the usual wait.
Thanks!


Segher