Hi!
On Wed, Aug 26, 2020 at 10:45:26PM -0400, Michael Meissner wrote:
> * config/rs6000/rs6000.md (FSCALAR): New mode iterator for floating
> point scalars.
We have the long-established SFDF for a very similar thing. So maybe
just call this SFDFTF or SDTF or something?
Alternatively, we have FP for all the floating point modes -- maybe
add a BFP that will then we just the (IEEE) binary floating point modes?
> (Fm): New mode attribute for floating point scalars.
Not happy about this at all. Just make *one* attribute that is the
constraint to use for any mode? And if there are multiple choices for
what constraint to use with some mode, well, hiding that behind a mode
attribute is obfuscation then.
The <Fv> attribute always is "wa" nowadays (I should clean that up some
day, heh). And <Ff> is either "f" or "d", but those two constraints are
identical these days.
I have no idea what "F" means? (Or "m"!)
> (s<minmax><mode>): Add support for the ISA 3.1 IEEE 128-bit
> minimum and maximum instructions.
> (s<minmax><mode>3_vsx): Add support for the ISA 3.1 IEEE 128-bit
> minimum and maximum instructions.
> +/* Macro whether the float128 min/max instructions are enabled. */
> +#define FLOAT128_IEEE_MINMAX_P(MODE) \
> + (TARGET_POWER10 && TARGET_FLOAT128_HW && FLOAT128_IEEE_P (MODE))
This is a horrible name. It is about the minmax*c* insns. And the "_P"
is wrong (this macro is not checking if the mode is a minmax, whatever
that would mean!)
> +;; Constraints to use for scalar FP operations
> +(define_mode_attr Fm [(SF "wa")
> + (DF "wa")
> + (TF "v")
> + (KF "v")])
You miss a lot of context here... This of course is *not* for all
scalar FP ops. Ah, so the "m" is for "minmax" I guess.
> (define_insn "*s<minmax><mode>3_vsx"
> - [(set (match_operand:SFDF 0 "vsx_register_operand" "=<Fv>")
> - (fp_minmax:SFDF (match_operand:SFDF 1 "vsx_register_operand" "<Fv>")
> - (match_operand:SFDF 2 "vsx_register_operand" "<Fv>")))]
> + [(set (match_operand:FSCALAR 0 "vsx_register_operand" "=<Fm>")
> + (fp_minmax:FSCALAR
> + (match_operand:FSCALAR 1 "vsx_register_operand" "<Fm>")
> + (match_operand:FSCALAR 2 "vsx_register_operand" "<Fm>")))]
> "TARGET_VSX && TARGET_HARD_FLOAT"
> {
> + if (FLOAT128_IEEE_P (<MODE>mode))
> + return "xs<minmax>cqp %0,%1,%2";
Nope. Since you need to handle this with completely separate code
anyway, just make it a separate instruction pattern please. And there
is no need for a define_expand for it, so you can just leave that one
intact as-is.
Segher
