Re: [PATCH 2/4] PowerPC: Rename functions for min, max, cmove
On Wed, Aug 26, 2020 at 10:44:22PM -0400, Michael Meissner wrote: > PowerPC: Rename functions for min, max, cmove. > > This patch renames the functions that generate the ISA 3.0 C minimum, C > maximum, and conditional move instructions to use a better name than just > using > a _p9 suffix. Because the functions can fail, the names use "maybe_emit" > instead of "generate_" in the name. Please keep the rs6000_ prefix though. If we want to drop that, we should do it *everywhere*. > -/* ISA 3.0 (power9) minmax subcase to emit a XSMAXCDP or XSMINCDP instruction > - for SF/DF scalars. Move TRUE_COND to DEST if OP of the operands of the > last > - comparison is nonzero/true, FALSE_COND if it is zero/false. Return 0 if > the > - hardware has no such operation. */ > +/* Possibly emit the C variant of the minimum or maximum instruction for > + floating point scalars (xsmincdp, xsmaxcdp, etc.). Please explain that this is asymmetric. This isn't a proper min/max, except with fastmath :-( > + /* See if we can use the "C" minimum, "C" maximum, and compare and set mask > + instructions. */ Just write the actual mnemonics please. Okay for trunk with those things fixed. Thanks! Segher
Re: [PATCH 2/4] PowerPC: Rename functions for min, max, cmove
On Thu, Aug 27, 2020 at 03:47:15PM -0500, will schmidt wrote: > On Wed, 2020-08-26 at 22:44 -0400, Michael Meissner via Gcc-patches wrote: > > * config/rs6000/rs6000.c (rs6000_emit_p9_fp_minmax): Rename to > > maybe_emit_fp_c_minmax. > ok > > > (maybe_emit_fp_c_minmax): Rename rs6000_emit_p9_fp_minmax. Return > > bool instead of int. > > function rename is redundant between the two entries? Yes, there are cleaner ways to write this, while still keeping both the old and new names on the lhs of the colon (where they should be!) Without naming each thing twice, even. Segher
Re: [PATCH 2/4] PowerPC: Rename functions for min, max, cmove
On Wed, 2020-08-26 at 22:44 -0400, Michael Meissner via Gcc-patches wrote:
> PowerPC: Rename functions for min, max, cmove.
>
> This patch renames the functions that generate the ISA 3.0 C minimum, C
> maximum, and conditional move instructions to use a better name than just
> using
> a _p9 suffix. Because the functions can fail, the names use "maybe_emit"
> instead of "generate_" in the name.
>
> I have built compilers on a little endian power9 Linux system with all 4
> patches applied. I did bootstrap builds and ran the testsuite, with no
> regressions. Previous versions of the patch was also tested on a little
> endian
> power8 Linux system. I would like to check this patch into the master
> branch for GCC 11. At this time, I do not anticipate needing to backport
> these
> changes to GCC 10.3.
>
> gcc/
> 2020-08-26 Michael Meissner
>
> * config/rs6000/rs6000.c (rs6000_emit_p9_fp_minmax): Rename to
> maybe_emit_fp_c_minmax.
ok
> (maybe_emit_fp_c_minmax): Rename rs6000_emit_p9_fp_minmax. Return
> bool instead of int.
function rename is redundant between the two entries?
> (rs6000_emit_p9_fp_cmove): Rename to maybe_emit_fp_cmove.
> (maybe_emit_fp_cmove): Rename rs6000_emit_p9_fp_cmove. Return
> bool instead of int.
Function rename comment is redundant ?
> (have_compare_and_set_mask): New helper function.
> (rs6000_emit_cmove): Rework support of ISA 3.0 functions to
> generate "C" minimum, "C" maximum, and conditional move
> instructions for scalar floating point.
>
Nothing else jumped out at me below.
lgtm, thanks,
-Will
> ---
> gcc/config/rs6000/rs6000.c | 77 ++
> 1 file changed, 53 insertions(+), 24 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index bac50c2bcf6..6324f930628 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -15056,13 +15056,17 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx
> op_true, rtx op_false,
>return 1;
> }
>
> -/* ISA 3.0 (power9) minmax subcase to emit a XSMAXCDP or XSMINCDP instruction
> - for SF/DF scalars. Move TRUE_COND to DEST if OP of the operands of the
> last
> - comparison is nonzero/true, FALSE_COND if it is zero/false. Return 0 if
> the
> - hardware has no such operation. */
> +/* Possibly emit the C variant of the minimum or maximum instruction for
> + floating point scalars (xsmincdp, xsmaxcdp, etc.).
>
> -static int
> -rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx true_cond, rtx false_cond)
> + Move TRUE_COND to DEST if OP of the operands of the last comparison is
> + nonzero/true, FALSE_COND if it is zero/false.
> +
> + Return false if we can't generate the appropriate minimum or maximum, and
> + true if we can did the minimum or maximum. */
> +
> +static bool
> +maybe_emit_fp_c_minmax (rtx dest, rtx op, rtx true_cond, rtx false_cond)
> {
>enum rtx_code code = GET_CODE (op);
>rtx op0 = XEXP (op, 0);
> @@ -15072,14 +15076,14 @@ rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx
> true_cond, rtx false_cond)
>bool max_p = false;
>
>if (result_mode != compare_mode)
> -return 0;
> +return false;
>
>if (code == GE || code == GT)
> max_p = true;
>else if (code == LE || code == LT)
> max_p = false;
>else
> -return 0;
> +return false;
>
>if (rtx_equal_p (op0, true_cond) && rtx_equal_p (op1, false_cond))
> ;
> @@ -15092,19 +15096,23 @@ rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx
> true_cond, rtx false_cond)
> max_p = !max_p;
>
>else
> -return 0;
> +return false;
>
>rs6000_emit_minmax (dest, max_p ? SMAX : SMIN, op0, op1);
> - return 1;
> + return true;
> }
>
> -/* ISA 3.0 (power9) conditional move subcase to emit XSCMP{EQ,GE,GT,NE}DP and
> - XXSEL instructions for SF/DF scalars. Move TRUE_COND to DEST if OP of the
> - operands of the last comparison is nonzero/true, FALSE_COND if it is
> - zero/false. Return 0 if the hardware has no such operation. */
> +/* Possibly emit a floating point conditional move by generating a compare
> that
> + sets a mask instruction and a XXSEL select instruction.
>
> -static int
> -rs6000_emit_p9_fp_cmove (rtx dest, rtx op, rtx true_cond, rtx false_cond)
> + Move TRUE_COND to DEST if OP of the operands of the last comparison is
> + nonzero/true, FALSE_COND if it is zero/false.
> +
> + Return false if the operation cannot be generated, and true if we could
> + generate the instruction. */
> +
> +static bool
> +maybe_emit_fp_cmove (rtx dest, rtx op, rtx true_cond, rtx false_cond)
> {
>enum rtx_code code = GET_CODE (op);
>rtx op0 = XEXP (op, 0);
> @@ -15132,7 +15140,7 @@ rs6000_emit_p9_fp_cmove (rtx dest, rtx op, rtx
> true_cond, rtx false_cond)
>break;
>
> default:
> - return 0;
> + return false;
> }
>
>/* Generate: [(parallel [(set (dest)
> @@
[PATCH 2/4] PowerPC: Rename functions for min, max, cmove
PowerPC: Rename functions for min, max, cmove.
This patch renames the functions that generate the ISA 3.0 C minimum, C
maximum, and conditional move instructions to use a better name than just using
a _p9 suffix. Because the functions can fail, the names use "maybe_emit"
instead of "generate_" in the name.
I have built compilers on a little endian power9 Linux system with all 4
patches applied. I did bootstrap builds and ran the testsuite, with no
regressions. Previous versions of the patch was also tested on a little endian
power8 Linux system. I would like to check this patch into the master
branch for GCC 11. At this time, I do not anticipate needing to backport these
changes to GCC 10.3.
gcc/
2020-08-26 Michael Meissner
* config/rs6000/rs6000.c (rs6000_emit_p9_fp_minmax): Rename to
maybe_emit_fp_c_minmax.
(maybe_emit_fp_c_minmax): Rename rs6000_emit_p9_fp_minmax. Return
bool instead of int.
(rs6000_emit_p9_fp_cmove): Rename to maybe_emit_fp_cmove.
(maybe_emit_fp_cmove): Rename rs6000_emit_p9_fp_cmove. Return
bool instead of int.
(have_compare_and_set_mask): New helper function.
(rs6000_emit_cmove): Rework support of ISA 3.0 functions to
generate "C" minimum, "C" maximum, and conditional move
instructions for scalar floating point.
---
gcc/config/rs6000/rs6000.c | 77 ++
1 file changed, 53 insertions(+), 24 deletions(-)
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index bac50c2bcf6..6324f930628 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -15056,13 +15056,17 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx op_true,
rtx op_false,
return 1;
}
-/* ISA 3.0 (power9) minmax subcase to emit a XSMAXCDP or XSMINCDP instruction
- for SF/DF scalars. Move TRUE_COND to DEST if OP of the operands of the last
- comparison is nonzero/true, FALSE_COND if it is zero/false. Return 0 if the
- hardware has no such operation. */
+/* Possibly emit the C variant of the minimum or maximum instruction for
+ floating point scalars (xsmincdp, xsmaxcdp, etc.).
-static int
-rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx true_cond, rtx false_cond)
+ Move TRUE_COND to DEST if OP of the operands of the last comparison is
+ nonzero/true, FALSE_COND if it is zero/false.
+
+ Return false if we can't generate the appropriate minimum or maximum, and
+ true if we can did the minimum or maximum. */
+
+static bool
+maybe_emit_fp_c_minmax (rtx dest, rtx op, rtx true_cond, rtx false_cond)
{
enum rtx_code code = GET_CODE (op);
rtx op0 = XEXP (op, 0);
@@ -15072,14 +15076,14 @@ rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx
true_cond, rtx false_cond)
bool max_p = false;
if (result_mode != compare_mode)
-return 0;
+return false;
if (code == GE || code == GT)
max_p = true;
else if (code == LE || code == LT)
max_p = false;
else
-return 0;
+return false;
if (rtx_equal_p (op0, true_cond) && rtx_equal_p (op1, false_cond))
;
@@ -15092,19 +15096,23 @@ rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx
true_cond, rtx false_cond)
max_p = !max_p;
else
-return 0;
+return false;
rs6000_emit_minmax (dest, max_p ? SMAX : SMIN, op0, op1);
- return 1;
+ return true;
}
-/* ISA 3.0 (power9) conditional move subcase to emit XSCMP{EQ,GE,GT,NE}DP and
- XXSEL instructions for SF/DF scalars. Move TRUE_COND to DEST if OP of the
- operands of the last comparison is nonzero/true, FALSE_COND if it is
- zero/false. Return 0 if the hardware has no such operation. */
+/* Possibly emit a floating point conditional move by generating a compare that
+ sets a mask instruction and a XXSEL select instruction.
-static int
-rs6000_emit_p9_fp_cmove (rtx dest, rtx op, rtx true_cond, rtx false_cond)
+ Move TRUE_COND to DEST if OP of the operands of the last comparison is
+ nonzero/true, FALSE_COND if it is zero/false.
+
+ Return false if the operation cannot be generated, and true if we could
+ generate the instruction. */
+
+static bool
+maybe_emit_fp_cmove (rtx dest, rtx op, rtx true_cond, rtx false_cond)
{
enum rtx_code code = GET_CODE (op);
rtx op0 = XEXP (op, 0);
@@ -15132,7 +15140,7 @@ rs6000_emit_p9_fp_cmove (rtx dest, rtx op, rtx
true_cond, rtx false_cond)
break;
default:
- return 0;
+ return false;
}
/* Generate: [(parallel [(set (dest)
@@ -15152,7 +15160,28 @@ rs6000_emit_p9_fp_cmove (rtx dest, rtx op, rtx
true_cond, rtx false_cond)
emit_insn (gen_rtx_PARALLEL (VOIDmode,
gen_rtvec (2, cmove_rtx, clobber_rtx)));
- return 1;
+ return true;
+}
+
+/* Helper function to return true if the target has instructions to do a
+ compare and set mask instruction that can be used with XXSEL to implement a
+ conditional move. It is also assumed that such a target also supports
