public inbox for
 help / color / mirror / Atom feed
From: Segher Boessenkool <>
To: Michael Meissner <>,, "Kewen.Lin" <>,
	David Edelsohn <>,
	Peter Bergner <>,
	Will Schmidt <>
Subject: Re: [PATCH] Improve converting between 128-bit modes that use the same format
Date: Tue, 23 Aug 2022 16:13:45 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

> -; 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")))]
> +{
> +  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")))]
> +{
> +  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!


  reply	other threads:[~2022-08-23 21:14 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-18 21:48 Michael Meissner
2022-08-23 21:13 ` Segher Boessenkool [this message]
2022-09-02 17:51   ` Michael Meissner
2022-09-06 22:22     ` Segher Boessenkool
2022-09-07 20:25       ` Michael Meissner
2022-09-07 20:56         ` Segher Boessenkool
2022-09-12 19:28 ` Michael Meissner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).