Tamar Christina <tamar.christ...@arm.com> writes:
>> > +     After the final loads are done it issues a
>> > +     vec_construct to recreate the vector from the scalar.  For costing 
>> > when
>> > +     we see a vec_to_scalar on a stmt with VMAT_GATHER_SCATTER we are
>> dealing
>> > +     with an emulated instruction and should adjust costing properly.  */
>> > +  if (kind == vec_to_scalar
>> > +      && (m_vec_flags & VEC_ADVSIMD)
>> > +      && vect_mem_access_type (stmt_info, node) == VMAT_GATHER_SCATTER)
>> > +    {
>> > +      auto dr = STMT_VINFO_DATA_REF (stmt_info);
>> > +      tree dr_ref = DR_REF (dr);
>> > +      /* Only really expect MEM_REF or ARRAY_REF here.  Ignore the rest.  
>> > */
>> > +      switch (TREE_CODE (dr_ref))
>> > +  {
>> > +  case MEM_REF:
>> > +  case ARRAY_REF:
>> > +  case TARGET_MEM_REF:
>> > +    {
>> > +      tree offset = TREE_OPERAND (dr_ref, 1);
>> > +      if (SSA_VAR_P (offset)
>> > +          && gimple_vuse (SSA_NAME_DEF_STMT (offset)))
>> > +        {
>> > +          if (STMT_VINFO_TYPE (stmt_info) == load_vec_info_type)
>> > +            ops->loads += count - 1;
>> > +          else
>> > +            /* Stores want to count both the index to array and data to
>> > +               array using vec_to_scalar.  However we have index stores in
>> > +               Adv.SIMD and so we only want to adjust the index loads.  */
>> > +            ops->loads += count / 2;
>> > +          return;
>> > +        }
>> > +      break;
>> > +    }
>> > +  default:
>> > +    break;
>> > +  }
>> > +    }
>> 
>> Operand 1 of MEM_REF and TARGET_MEM_REF are always constant, so the
>> handling of those codes looks redundant.  Perhaps we should instead use:
>> 
>>    while (handled_component_p (dr_ref))
>>      {
>>        if (TREE_CODE (dr_ref) == ARRAY_REF)
>>          {
>>          ...early return or break if SSA offset found...
>>          }
>>        dr_ref = TREE_OPERAND (dr_ref, 0);
>>      }
>> 
>> A gather load could reasonably be to COMPONENT_REFs or BIT_FIELD_REFs of
>> an ARRAY_REF, so the ARRAY_REF might not be the outermost code.
>> 
>
> Ah, I tried to find their layouts but couldn't readily find one.  
> Interesting, I would have
> Thought a scatter/gather on either of those is more efficiently done through 
> bit masking.

It can happen in things like:

#define iterations 100000
#define LEN_1D 32000

typedef struct { float f; } floats;
float a[LEN_1D];
floats b[LEN_1D][2];

float
s4115 (int *ip)
{
    float sum = 0.;
    for (int i = 0; i < LEN_1D; i++)
      {
        sum += a[i] * b[ip[i]][0].f;
      }
    return sum;
}

where the outer compoennt reference is the ".f" and the array reference
is hidden inside.  This example also shows why I think we should only
break on SSA offsets: the outer array reference has a constant index,
but the inner one is variable and is loaded from memory.

So:

> Here's updated version of patch.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
>       PR target/118188
>       * config/aarch64/aarch64.cc (aarch64_vector_costs::count_ops): Adjust
>       throughput of emulated gather and scatters.
>
> gcc/testsuite/ChangeLog:
>
>       PR target/118188
>       * gcc.target/aarch64/sve/gather_load_12.c: New test.
>       * gcc.target/aarch64/sve/gather_load_13.c: New test.
>       * gcc.target/aarch64/sve/gather_load_14.c: New test.
>
> -- inline copy of patch --
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 
> 3e700ed41e97a98dc844cad1c8a66a3555d82221..6a7abceda466f5569d743b0d3a0eed1bbeb79a2c
>  100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -17378,6 +17378,46 @@ aarch64_vector_costs::count_ops (unsigned int count, 
> vect_cost_for_stmt kind,
>       return;
>      }
>  
> +  /* Detect the case where we are using an emulated gather/scatter.  When a
> +     target does not support gathers and scatters directly the vectorizer
> +     emulates these by constructing an index vector and then issuing an
> +     extraction for every lane in the vector.  If the index vector is loaded
> +     from memory, the vector load and extractions are subsequently lowered by
> +     veclower into a series of scalar index loads.  After the final loads are
> +     done it issues a vec_construct to recreate the vector from the scalar.  
> For
> +     costing when we see a vec_to_scalar on a stmt with VMAT_GATHER_SCATTER 
> we
> +     are dealing with an emulated instruction and should adjust costing
> +     properly.  */
> +  if (kind == vec_to_scalar
> +      && (m_vec_flags & VEC_ADVSIMD)
> +      && vect_mem_access_type (stmt_info, node) == VMAT_GATHER_SCATTER)
> +    {
> +      auto dr = STMT_VINFO_DATA_REF (stmt_info);
> +      tree dr_ref = DR_REF (dr);
> +      while (handled_component_p (dr_ref))
> +     {
> +       if (TREE_CODE (dr_ref) == ARRAY_REF)
> +         {
> +           tree offset = TREE_OPERAND (dr_ref, 1);
> +           if (SSA_VAR_P (offset)
> +               && gimple_vuse (SSA_NAME_DEF_STMT (offset)))
> +             {
> +               if (STMT_VINFO_TYPE (stmt_info) == load_vec_info_type)
> +                 ops->loads += count - 1;
> +               else
> +                 /* Stores want to count both the index to array and data to
> +                    array using vec_to_scalar.  However we have index stores
> +                    in Adv.SIMD and so we only want to adjust the index
> +                    loads.  */
> +                 ops->loads += count / 2;
> +               return;
> +           }
> +         else
> +           break;

...rather than break for all ARRAY_REFs, I think we should do:

          if (TREE_CODE (dr_ref) == ARRAY_REF)
            {
              tree offset = TREE_OPERAND (dr_ref, 1);
              if (SSA_VAR_P (offset))
                {
                  if (gimple_vuse (SSA_NAME_DEF_STMT (offset)))
                    {
                      if (STMT_VINFO_TYPE (stmt_info) == load_vec_info_type)
                        ops->loads += count - 1;
                      else
                        /* Stores want to count both the index to array and
                           data to array using vec_to_scalar.  However we have
                           index stores in Adv.SIMD and so we only want to
                           adjust the index loads.  */
                        ops->loads += count / 2;
                      return;
                    }
                  break;
                }
            }

OK with that change if you agree, and if it works.  (I did check the
above example locally FWIW.)

Thanks,
Richard

> +        }
> +       dr_ref = TREE_OPERAND (dr_ref, 0);
> +     }
> +    }
>  
>    /* Count the basic operation cost associated with KIND.  */
>    switch (kind)
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/gather_load_12.c 
> b/gcc/testsuite/gcc.target/aarch64/sve/gather_load_12.c
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..d550f005d638f260973312fbccd05e2bb9aef1a7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/gather_load_12.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-Ofast -mcpu=neoverse-v2" } */
> +
> +#define iterations 100000
> +#define LEN_1D 32000
> +
> +float a[LEN_1D], b[LEN_1D];
> +
> +float
> +s4115 (int *ip)
> +{
> +    float sum = 0.;
> +    for (int i = 0; i < LEN_1D; i++)
> +      {
> +        sum += a[i] * b[ip[i]];
> +      }
> +    return sum;
> +}
> +
> +/* { dg-final { scan-assembler {\s+ld1w\t} } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/gather_load_13.c 
> b/gcc/testsuite/gcc.target/aarch64/sve/gather_load_13.c
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..24da0646a75e41eae0c0dd6a2b8af58631ac94cb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/gather_load_13.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-Ofast -mcpu=neoverse-v2" } */
> +
> +#define iterations 100000
> +#define LEN_1D 32000
> +
> +float a[LEN_1D], b[LEN_1D];
> +
> +float
> +s4115 (int *ip)
> +{
> +    float sum = 0.;
> +    for (int i = 0; i < LEN_1D; i++)
> +      {
> +        sum += a[i] * b[ip[i] + 1];
> +      }
> +    return sum;
> +}
> +
> +/* { dg-final { scan-assembler {\s+ld1w\t} { xfail *-*-* } } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/gather_load_14.c 
> b/gcc/testsuite/gcc.target/aarch64/sve/gather_load_14.c
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..77d06d2a0e0ee89e5e45730eebf9b4bebfa2aaed
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/gather_load_14.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-Ofast -mcpu=neoverse-v2" } */
> +
> +#define iterations 100000
> +#define LEN_1D 32000
> +
> +float a[LEN_1D], b[LEN_1D];
> +
> +float
> +s4115 (int *ip)
> +{
> +    float sum = 0.;
> +    for (int i = 0; i < LEN_1D; i++)
> +      {
> +        sum += a[i] * b[ip[i]];
> +      }
> +    return sum;
> +}
> +
> +/* { dg-final { scan-assembler-not {\s+st1w\t} } } */

Reply via email to