[PATCH, rs6000] Disable gimple fold for float or double vec_minmax when fast-math is not set

Bill Schmidt wschmidt@linux.ibm.com
Tue Aug 24 20:04:26 GMT 2021


Hi Hao Chen,

On 8/24/21 3:52 AM, HAO CHEN GUI wrote:
> Hi
>
>      The patch disables gimple fold for float or double vec_min/max
> builtin when fast-math is not set. Two test cases are added to verify
> the patch.
>
>      The attachments are the patch diff and change log file.
>
>      Bootstrapped and tested on powerpc64le-linux with no regressions. Is
> this okay for trunk? Any recommendations? Thanks a lot.
>
Thanks for this patch!  In the future, if you can put your ChangeLog and 
patch inline in your post, it makes it easier to review.  (Otherwise we 
have to manually copy it into our response and manipulate it to look 
quoted, etc.)

Your ChangeLog isn't formatted correctly.  It should look like this:

2021-08-24  Hao Chen Gui  <guihaoc@linux.ibm.com>

gcc/
	* config/rs6000/rs6000-call.c (rs6000_gimple_fold_builtin): Modify the
	VSX_BUILTIN_XVMINDP, ALTIVEC_BUILTIN_VMINFP, VSX_BUILTIN_XVMAXDP, and
	ALTIVEC_BUILTIN_VMAXFP expansions.

gcc/testsuite/
	* gcc.target/powerpc/vec-minmax-1.c: New test.
	* gcc.target/powerpc/vec-minmax-2.c: Likewise.

You forgot the committer/timestamp line and the ChangeLog location 
lines.  (The headers like "gcc/" ensure that the automated processing 
will record your entries in the ChangeLog at the correct location in the 
source tree.)  Note also that the colon ":" always follows the ending 
parenthesis when there's a function name listed.  Please review 
https://gcc.gnu.org/codingconventions.html#ChangeLogs.

> diff --git a/gcc/config/rs6000/rs6000-call.c 
> b/gcc/config/rs6000/rs6000-call.c index b4e13af4dc6..90527734ceb 
> 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ 
> b/gcc/config/rs6000/rs6000-call.c @@ -12159,6 +12159,11 @@ 
> rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) return true; /* 
> flavors of vec_min. */ case VSX_BUILTIN_XVMINDP: + case 
> ALTIVEC_BUILTIN_VMINFP: + if (!flag_finite_math_only || 
> flag_signed_zeros) + return false; + /* Fall through to MIN_EXPR. */ + 
> gcc_fallthrough (); case P8V_BUILTIN_VMINSD: case P8V_BUILTIN_VMINUD: 
> case ALTIVEC_BUILTIN_VMINSB: @@ -12167,7 +12172,6 @@ 
> rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) case 
> ALTIVEC_BUILTIN_VMINUB: case ALTIVEC_BUILTIN_VMINUH: case 
> ALTIVEC_BUILTIN_VMINUW: - case ALTIVEC_BUILTIN_VMINFP: arg0 = 
> gimple_call_arg (stmt, 0); arg1 = gimple_call_arg (stmt, 1); lhs = 
> gimple_call_lhs (stmt); @@ -12177,6 +12181,11 @@ 
> rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) return true; /* 
> flavors of vec_max. */ case VSX_BUILTIN_XVMAXDP: + case 
> ALTIVEC_BUILTIN_VMAXFP: + if (!flag_finite_math_only || 
> flag_signed_zeros) + return false; + /* Fall through to MAX_EXPR. */ + 
> gcc_fallthrough (); case P8V_BUILTIN_VMAXSD: case P8V_BUILTIN_VMAXUD: 
> case ALTIVEC_BUILTIN_VMAXSB: @@ -12185,7 +12194,6 @@ 
> rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) case 
> ALTIVEC_BUILTIN_VMAXUB: case ALTIVEC_BUILTIN_VMAXUH: case 
> ALTIVEC_BUILTIN_VMAXUW: - case ALTIVEC_BUILTIN_VMAXFP: arg0 = 
> gimple_call_arg (stmt, 0); arg1 = gimple_call_arg (stmt, 1); lhs = 
> gimple_call_lhs (stmt); diff --git 
> a/gcc/testsuite/gcc.target/powerpc/vec-minmax-1.c 
> b/gcc/testsuite/gcc.target/powerpc/vec-minmax-1.c new file mode 100644 
> index 00000000000..9782d1b9308 --- /dev/null +++ 
> b/gcc/testsuite/gcc.target/powerpc/vec-minmax-1.c @@ -0,0 +1,51 @@ +/* 
> { dg-do compile { target { powerpc64le-*-* } } } */ +/* { 
> dg-require-effective-target powerpc_p9vector_ok } */ +/* { dg-options 
> "-O2 -mdejagnu-cpu=power9" } */ +/* { dg-final { scan-assembler-times 
> {\mxvmax[ds]p\M} 2 } } */ +/* { dg-final { scan-assembler-times 
> {\mxvmin[ds]p\M} 2 } } */ 

This is pedantic, but...  You want exactly one each of xvmaxdp, xvmaxsp, xvmindp, and xvminsp,
so please replace this with four lines with { scan-assembler-times {...} 1 }.  Thanks. :-)

Otherwise this looks fine to me.  I can't approve, but recommend the maintainers approve with
that changed.

Thanks!
Bill
> + +/* This test verifies that float or double vec_min/max are bound to 
> + xv[min|max][d|s]p instructions when fast-math is not set. */ + + 
> +#include <altivec.h> + +#ifdef _BIG_ENDIAN + const int PREF_D = 0; 
> +#else + const int PREF_D = 1; +#endif + +double vmaxd (double a, 
> double b) +{ + vector double va = vec_promote (a, PREF_D); + vector 
> double vb = vec_promote (b, PREF_D); + return vec_extract (vec_max 
> (va, vb), PREF_D); +} + +double vmind (double a, double b) +{ + vector 
> double va = vec_promote (a, PREF_D); + vector double vb = vec_promote 
> (b, PREF_D); + return vec_extract (vec_min (va, vb), PREF_D); +} + 
> +#ifdef _BIG_ENDIAN + const int PREF_F = 0; +#else + const int PREF_F 
> = 3; +#endif + +float vmaxf (float a, float b) +{ + vector float va = 
> vec_promote (a, PREF_F); + vector float vb = vec_promote (b, PREF_F); 
> + return vec_extract (vec_max (va, vb), PREF_F); +} + +float vminf 
> (float a, float b) +{ + vector float va = vec_promote (a, PREF_F); + 
> vector float vb = vec_promote (b, PREF_F); + return vec_extract 
> (vec_min (va, vb), PREF_F); +} diff --git 
> a/gcc/testsuite/gcc.target/powerpc/vec-minmax-2.c 
> b/gcc/testsuite/gcc.target/powerpc/vec-minmax-2.c new file mode 100644 
> index 00000000000..d318b933181 --- /dev/null +++ 
> b/gcc/testsuite/gcc.target/powerpc/vec-minmax-2.c @@ -0,0 +1,51 @@ +/* 
> { dg-do compile { target { powerpc64le-*-* } } } */ +/* { 
> dg-require-effective-target powerpc_p9vector_ok } */ +/* { dg-options 
> "-O2 -mdejagnu-cpu=power9 -ffast-math" } */ +/* { dg-final { 
> scan-assembler-times {\mxsmaxcdp\M} 2 } } */ +/* { dg-final { 
> scan-assembler-times {\mxsmincdp\M} 2 } } */ + +/* This test verifies 
> that float or double vec_min/max can be converted + to scalar 
> comparison when fast-math is set. */ + + +#include <altivec.h> + 
> +#ifdef _BIG_ENDIAN + const int PREF_D = 0; +#else + const int PREF_D 
> = 1; +#endif + +double vmaxd (double a, double b) +{ + vector double 
> va = vec_promote (a, PREF_D); + vector double vb = vec_promote (b, 
> PREF_D); + return vec_extract (vec_max (va, vb), PREF_D); +} + +double 
> vmind (double a, double b) +{ + vector double va = vec_promote (a, 
> PREF_D); + vector double vb = vec_promote (b, PREF_D); + return 
> vec_extract (vec_min (va, vb), PREF_D); +} + +#ifdef _BIG_ENDIAN + 
> const int PREF_F = 0; +#else + const int PREF_F = 3; +#endif + +float 
> vmaxf (float a, float b) +{ + vector float va = vec_promote (a, 
> PREF_F); + vector float vb = vec_promote (b, PREF_F); + return 
> vec_extract (vec_max (va, vb), PREF_F); +} + +float vminf (float a, 
> float b) +{ + vector float va = vec_promote (a, PREF_F); + vector 
> float vb = vec_promote (b, PREF_F); + return vec_extract (vec_min (va, 
> vb), PREF_F); +} 



More information about the Gcc-patches mailing list