Re: [PATCH] vect, omp: inbranch simdclone dropping const

[Andrew Stubbs]

I don't have authority to approve anything, but here's a review anyway.
Thanks for working on this.

On 26/09/2023 17:24, Andre Vieira (lists) wrote:
The const attribute is ignored when simdclone's are used inbranch. This 
is due to the fact that when analyzing a MASK_CALL we were not looking 
at the targeted function for flags, but instead only at the internal 
function call itself.
This patch adds code to make sure we look at the target function to 
check for the const attribute and enables the autovectorization of 
inbranch const simdclones without needing the loop to be adorned the 
'openmp simd' pragma.

Not sure about how to add new includes to the ChangeLog. Which brings me 
to another point, I contemplated changing gimple_call_flags to do the 
checking of flags of the first argument of IFN_MASK_CALL itself rather 
than only calling internal_fn_flags on gimple_call_internal_fn (stmt), 
but that might be a bit too intrusive, opinions welcome :)

Bootstrapped and regression tested on aarch64-unknown-linux-gnu and 
x86_64-pc-linux-gnu.

Is this OK for trunk?

gcc/ChangeLog:

         * tree-vect-data-ref.cc (include calls.h): Add new include.
         (get_references_in_stmt): Correctly handle IFN_MASK_CALL.

gcc/testsuite/ChangeLog:

         * gcc.dg/vect/vect-simd-clone-19.c: New test.

diff --git a/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c 
b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c
new file mode 100644
index 
0000000000000000000000000000000000000000..09127b8cb6f2e3699b6073591f58be7047330273
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vect/vect-simd-clone-19.c
@@ -0,0 +1,23 @@
+/* { dg-require-effective-target vect_simd_clones } */
+/* { dg-do compile } */
+/* { dg-additional-options "-fopenmp-simd" } */
+

Do you need -fopenmp-simd for this?

+             tree orig_fndecl
+               = gimple_call_addr_fndecl (gimple_call_arg (stmt, 0));
+             if (!orig_fndecl)
+               {
+                 clobbers_memory = true;
+                 break;
+               }
+             if ((flags_from_decl_or_type (orig_fndecl) & ECF_CONST) == 0)
+               clobbers_memory = true;
+           }
            break;
 

Can be simplified:

  if (!orig_fndecl
      || (flags_from_decl_or_type (orig_fndecl) & ECF_CONST) == 0)
    clobbers_memory = true;
  break;

@@ -5852,7 +5865,7 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, 
va_heap> *references)
     }
   else if (stmt_code == GIMPLE_CALL)
     {
-      unsigned i, n;
+      unsigned i  = 0, n;
       tree ptr, type;
       unsigned int align;
 

Rogue space.

@@ -5879,13 +5892,15 @@ get_references_in_stmt (gimple *stmt, vec<data_ref_loc, 
va_heap> *references)
                                   ptr);
            references->safe_push (ref);
            return false;
+         case IFN_MASK_CALL:
+           i = 1;
          default:
            break;
          }
 

If the fall-through is deliberate please add a /* FALLTHROUGH */ comment 
(or whatever spelling disables the warning).

Andrew