Re: [PATCH 1/2] Add IEEE 128-bit min/max support on PowerPC

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

Hi!

On Fri, Apr 09, 2021 at 11:54:57AM -0500, will schmidt wrote:
> On Fri, 2021-04-09 at 10:42 -0400, Michael Meissner wrote:
> > 	* config/rs6000/rs6000.c (rs6000_emit_minmax): Add support for ISA
> > 	3.1 IEEE 128-bit floating point xsmaxcqp and xsmincqp instructions.
> 
> I don't see any direct reference to xsmaxcqp or xsmincqp with respect
> to this change below. 
> 
> It looks like this change adds the FLOAT128_MIN_MAX_FPMASK_P (mode)
> check
> as criteria for emitting some form of a SET instruction. 
>        emit_insn (gen_rtx_SET (dest, gen_rtx_fmt_ee (code, mode, op0, op1)));
> 
> Ok, I see it now,  the instructions are mildly obfuscated by
> "xs<minmax>cqp" as part of the rs6000.md change below.

Yeah, that is the downside of using iterators and the like in the
machine description.  But the upsides of that far outweigh these
downsides :-)

> > 	* config/rs6000/rs60000.h (FLOAT128_MIN_MAX_FPMASK_P): New macro.
> 
> which is
> #define FLOAT128_MIN_MAX_FPMASK_P(MODE)	\
>   (TARGET_POWER10 && TARGET_FLOAT128_HW && FLOAT128_IEEE_P (MODE))
> 
> Are there any non MIN_MAX scenarios that will require the combination
> of POWER10,FLOAT128_HW,FLOAT128_IEEE(mode)?  I'd wonder if there is a name
> not specific to *_MIN_MAX_* that would be a better naming choice.
> But, naming is hard. :-)

Yes, and for every new macro, the reader will have to understand what it
does, what it is for.  If you cannot come up with a good name for it
(one for which it is immediately obvious to the reader what it *means*),
often a good tradeoff is to not make a macro at all, just write out the
few words where you need them.

> > 	* config/rs6000/rs6000.md (s<minmax><mode>3): Add support for the
> > 	ISA 3.1 IEEE 128-bit minimum and maximum instructions.
> 
> I'd move the "xsmaxcqp,xsmincqp" instruction references from the rs6000.c changelog blurb to this changelog blurb.

It should say "New." or "New define_insns." or "New instructions." or
similar.  The changelog says *what*, not *why*.  And it is important
that you can find stuff in it using grep or similar.

Here it should say "s<minmax><mode>3 for IEEE128".  We actually have
some patterns that just say "<code><mode>3", not too useful in a
changelog if you do not say what code and mode are!  (In this case, it
does not help to say what "minmax" is from, it stand for just "min" and
"max" after all :-) )


Segher