Re: [PATCH] Improve converting between 128-bit modes that use the same format

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

Please do not send new patches as replies to other patches.

On Thu, Aug 18, 2022 at 05:48:29PM -0400, Michael Meissner wrote:
> mprove converting between 128-bit modes that use the same format.

You are missing some characters?  But this is an edited version of the
subject anyway.  Just don't do that (neither the copying or the
editing), it just confuses things.

Please factor this patch into more pieces, pieces that can be reviewed
more easily, pieces that change one thing only.

As is you are just rewriting the lot, and it is not an improvement at
all this way.  No doubt there are many good pieces in it, but mixed with
a non-trivial amount of bad pieces I cannot approve it.  It also isn't
clear at all what you want to do; piece by piece it is easier to
explain.

> -; Iterators for converting to/from TFmode
> -(define_mode_iterator IFKF [IF KF])

Yes, IFmode and KFmode have almost nothing in common.  Good to see this
go.  It would be even better if we would not use
rs6000_expand_float128_convert when not needed, either, and all this
would be just gone after expand.

> +(define_expand "extendkfif2"
> +  [(set (match_operand:IF 0 "gpc_reg_operand")
> +	(float_extend:IF (match_operand:KF 1 "gpc_reg_operand")))]
> +  "TARGET_FLOAT128_TYPE"
> +{
> +  rs6000_expand_float128_convert (operands[0], operands[1], false);
> +  DONE;
> +})

This does not belong here.

It really shouldn't *exist* at all: this is *not* a float_extend!  It is
not converting to a wider mode (as required!), but not even to a mode of
higher precision: both IFmode and KFmode can represent (finite, normal)
numbers the other can not.

But it certainly does not belong here in the middle of no-op moves.

> +(define_expand "trunckfif2"
> +  [(set (match_operand:IF 0 "gpc_reg_operand")
> +	(float_truncate:IF (match_operand:KF 1 "gpc_reg_operand")))]
> +  "TARGET_FLOAT128_TYPE"
> +{
> +  rs6000_expand_float128_convert (operands[0], operands[1], false);
> +  DONE;
> +})

I also would expect IBM128 instead of just IF.  This would simplify a
lot.  Why do you not use that, is there a reason?

> +;; Convert between KFmode and TFmode when -mabi=ieeelongdouble
> +(define_insn_and_split "*extendkftf2_internal"

Same for IEEE128.  And this isn't a conversion at all (it's a no-op
move), please don't confuse things by saying it is.

> +  [(set_attr "type" "two")
> +   (set_attr "num_insns" "2")])

Btw, that really should never be needed.  insn_type "two" already means
exactly that.

> +(define_insn_and_split "*extendtfif2_internal"
> +  [(set (match_operand:IF 0 "gpc_reg_operand" "=d,&d")
> +	(float_extend:IF
> +	 (match_operand:TF 1 "input_operand" "0,d")))]
> +   "FLOAT128_IBM_P (TFmode)"
> +  "#"
> +  "&& reload_completed"

Why would this ever need reload_completed?  It is a no-op move!


Segher