Hi Kewen,

Sorry this sat in my queue for so long.  It looks like you addressed all of our concerns, so LGTM -- recommend maintainers approve.

Thanks!
Bill

On 8/12/21 9:34 PM, Kewen.Lin wrote:
Hi Segher,

Thanks for the review!

on 2021/8/12 下午11:10, Segher Boessenkool wrote:
Hi!

On Wed, Aug 11, 2021 at 02:56:11PM +0800, Kewen.Lin wrote:
        * config/rs6000/rs6000.c (rs6000_builtin_md_vectorized_function): Add
        support for some built-in functions vectorized on Power10.
Say which, not "some" please?

Done.

+  machine_mode in_vmode = TYPE_MODE (type_in);
+  machine_mode out_vmode = TYPE_MODE (type_out);
+
+  /* Power10 supported vectorized built-in functions.  */
+  if (TARGET_POWER10
+      && in_vmode == out_vmode
+      && VECTOR_UNIT_ALTIVEC_OR_VSX_P (in_vmode))
+    {
+      machine_mode exp_mode = DImode;
+      machine_mode exp_vmode = V2DImode;
+      enum rs6000_builtins vname = RS6000_BUILTIN_COUNT;
"name"?  This should be "bif" or similar?

Updated with name.

+      switch (fn)
+       {
+       case MISC_BUILTIN_DIVWE:
+       case MISC_BUILTIN_DIVWEU:
+         exp_mode = SImode;
+         exp_vmode = V4SImode;
+         if (fn == MISC_BUILTIN_DIVWE)
+           vname = P10V_BUILTIN_DIVES_V4SI;
+         else
+           vname = P10V_BUILTIN_DIVEU_V4SI;
+         break;
+       case MISC_BUILTIN_DIVDE:
+       case MISC_BUILTIN_DIVDEU:
+         if (fn == MISC_BUILTIN_DIVDE)
+           vname = P10V_BUILTIN_DIVES_V2DI;
+         else
+           vname = P10V_BUILTIN_DIVEU_V2DI;
+         break;
All of the above should not be builtin functions really, they are all
simple arithmetic :-(  They should not be UNSPECs either, on RTL level.
They can and should be optimised in real code as well.  Oh well.

--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/dive-vectorize-2.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target lp64 } */
Please add a comment what this is needed for?  "We scan for dive*d" is
enough, but without anything, it takes time to figure this out.

Done, same for below requests on lp64 commentary.

--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/dive-vectorize-run-2.c
@@ -0,0 +1,53 @@
+/* { dg-do run } */
+/* { dg-require-effective-target lp64 } */
Same here.  I suppose this uses builtins that do not exist on 32-bit?

Yeah, those bifs which are guarded with lp64 in their cases are only
supported on 64-bit environment.

--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/p10-bifs-vectorize-run-1.c
@@ -0,0 +1,45 @@
+/* { dg-do run } */
+/* { dg-require-effective-target lp64 } */
And another.

+#define CHECK(name)                                                           \
+  __attribute__ ((optimize (1))) void check_##name ()                         \
What is the attribute for, btw?  It seems fragile, but perhaps I do not
understand the intention.


It's to stop compiler from optimizing check functions with vectorization,
since the test point is to compare the results between scalar and vectorized
version.

Okay for trunk with whose lp64 things improved.  Thanks!

Thanks, v2 has been attached by addressing Bill's and your comments.  :)


BR,
Kewen
-----
gcc/ChangeLog:

        * config/rs6000/rs6000.c (rs6000_builtin_md_vectorized_function): Add
        support for built-in functions MISC_BUILTIN_DIVWE, MISC_BUILTIN_DIVWEU,
        MISC_BUILTIN_DIVDE, MISC_BUILTIN_DIVDEU, P10_BUILTIN_CFUGED,
        P10_BUILTIN_CNTLZDM, P10_BUILTIN_CNTTZDM, P10_BUILTIN_PDEPD and
        P10_BUILTIN_PEXTD on Power10.

Reply via email to