RE: [PATCH v1] Internal-fn: Handle vector bool type for type strict match mode [PR116103]

[Li, Pan2]

Thanks Richard S for comments, updated in v2.

https://gcc.gnu.org/pipermail/gcc-patches/2024-July/658637.html

Pan

-----Original Message-----
From: Richard Sandiford <richard.sandif...@arm.com> 
Sent: Tuesday, July 30, 2024 12:09 AM
To: Li, Pan2 <pan2...@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com; 
richard.guent...@gmail.com; tamar.christ...@arm.com; jeffreya...@gmail.com; 
rdapp....@gmail.com
Subject: Re: [PATCH v1] Internal-fn: Handle vector bool type for type strict 
match mode [PR116103]

pan2...@intel.com writes:
> From: Pan Li <pan2...@intel.com>
>
> For some target like target=amdgcn-amdhsa,  we need to take care of
> vector bool types prior to general vector mode types.  Or we may have
> the asm check failure as below.
>
> gcc.target/gcn/cond_smax_1.c scan-assembler-times \\tv_cmp_gt_i32\\tvcc, 
> s[0-9]+, v[0-9]+ 80
> gcc.target/gcn/cond_smin_1.c scan-assembler-times \\tv_cmp_gt_i32\\tvcc, 
> s[0-9]+, v[0-9]+ 80
> gcc.target/gcn/cond_umax_1.c scan-assembler-times \\tv_cmp_gt_i32\\tvcc, 
> s[0-9]+, v[0-9]+ 56
> gcc.target/gcn/cond_umin_1.c scan-assembler-times \\tv_cmp_gt_i32\\tvcc, 
> s[0-9]+, v[0-9]+ 56
> gcc.dg/tree-ssa/loop-bound-2.c scan-tree-dump-not ivopts "zero if "
>
> The below test suites are passed for this patch.
> 1. The rv64gcv fully regression tests.
> 2. The x86 bootstrap tests.
> 3. The x86 fully regression tests.
> 4. The amdgcn test case as above.
>
> gcc/ChangeLog:
>
>       * internal-fn.cc (type_strictly_matches_mode_p): Add handling
>       for vector bool type.
>
> Signed-off-by: Pan Li <pan2...@intel.com>
> ---
>  gcc/internal-fn.cc | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/gcc/internal-fn.cc b/gcc/internal-fn.cc
> index 8a2e07f2f96..086c8be398a 100644
> --- a/gcc/internal-fn.cc
> +++ b/gcc/internal-fn.cc
> @@ -4171,6 +4171,12 @@ direct_internal_fn_optab (internal_fn fn)
>  static bool
>  type_strictly_matches_mode_p (const_tree type)
>  {
> +  /* For target=amdgcn-amdhsa,  we need to take care of vector bool types.
> +     More details see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116103.  
> */
> +  if (VECTOR_BOOLEAN_TYPE_P (type) && SCALAR_INT_MODE_P (TYPE_MODE (type))
> +    && TYPE_PRECISION (TREE_TYPE (type)) == 1)

Sorry for the formatting nits, but I think this should be:

  if (VECTOR_BOOLEAN_TYPE_P (type)
      && SCALAR_INT_MODE_P (TYPE_MODE (type))
      && TYPE_PRECISION (TREE_TYPE (type)) == 1)

(one condition per line, indented below "VECTOR").

But I think the comment should give the underlying reason, rather than
treat it as a target oddity.  Maybe something like:

  /* Masked vector operations have both vector data operands and
     vector boolean operands.  The vector data operands are expected
     to have a vector mode, but the vector boolean operands can be
     an integer mode rather than a vector mode, depending on how
     TARGET_VECTORIZE_GET_MASK_MODE is defined.  */

Thanks,
Richard

> +    return true;
> +
>    if (VECTOR_TYPE_P (type))
>      return VECTOR_MODE_P (TYPE_MODE (type));