RE: RE: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic

["Li, Pan2" <pan2.li@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 10455 bytes --]

Got it, will have a try.

Pan

From: juzhe.zhong@rivai.ai <juzhe.zhong@rivai.ai>
Sent: Tuesday, September 12, 2023 9:30 AM
To: Li, Pan2 <pan2.li@intel.com>
Cc: kito.cheng <kito.cheng@gmail.com>; gcc-patches <gcc-patches@gcc.gnu.org>; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: Re: RE: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic

Add a function call get_non_overloaded_instance into instance.
The instance already know it is void vmv (void).
In this function search the arglist. and return the real non-overloaded decl.

________________________________
juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>

From: Li, Pan2<mailto:pan2.li@intel.com>
Date: 2023-09-12 09:20
To: 钟居哲<mailto:juzhe.zhong@rivai.ai>
CC: kito.cheng<mailto:kito.cheng@gmail.com>; gcc-patches<mailto:gcc-patches@gcc.gnu.org>; Wang, Yanzhang<mailto:yanzhang.wang@intel.com>
Subject: RE: RE: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic
We cannot leverage this instance for correctness.
The rfun of below code is the overloaded builtin is for the overloaded function, which is registered as void xxx(void) as aarch64 did to avoid the conflict.

Let’s take vmv_v_i32m1 as example in rfun table.

Index 0: void vmv_v(void) overloaded
Index 1: i32m1 vmv_v_v_i32m1_i32m1 (i32m1, size_t) non-overloaded
Index 2: placeholder.

When we enter the hook(aka the code list below), the rfun we have is the index 0 rfun instead of index 1.
Then we need the arglist to lookup the rfun of index 1 for the underlying call, as well as build the instance for the index 1 rfun.

Aarch64 has the same rfun table as above, they leverage a loop to parse the arglist with machine mode matching in a predefined type suffix(which is not available in RISC-V).

I think they almost try to resolve the same problem but different implement details.

Pan

From: 钟居哲 <juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>>
Sent: Tuesday, September 12, 2023 7:20 AM
To: Li, Pan2 <pan2.li@intel.com<mailto:pan2.li@intel.com>>
Cc: kito.cheng <kito.cheng@gmail.com<mailto:kito.cheng@gmail.com>>; gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>>; Wang, Yanzhang <yanzhang.wang@intel.com<mailto:yanzhang.wang@intel.com>>
Subject: Re: RE: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic

I don't understand.


+tree

+resolve_overloaded_builtin (location_t loc, unsigned int code,

+                         vec<tree, va_gc> *arglist)

+{

+  if (code >= vec_safe_length (registered_functions))

+    return NULL_TREE;

+

+  const registered_function *rfun = (*registered_functions)[code];

+

+  if (!rfun || !rfun->overloaded_p)

+    return NULL_TREE;

+

+  return function_resolver (loc, rfun->instance, rfun->decl, *arglist)

+    .resolve ();

+}
You already have rfun->instance. Just use this instance should be good enough.
________________________________
juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>

From: Li, Pan2<mailto:pan2.li@intel.com>
Date: 2023-09-11 23:24
To: 钟居哲<mailto:juzhe.zhong@rivai.ai>
CC: kito.cheng<mailto:kito.cheng@gmail.com>; gcc-patches<mailto:gcc-patches@gcc.gnu.org>; Wang, Yanzhang<mailto:yanzhang.wang@intel.com>
Subject: RE: RE: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic
For function instance with void or void arguments, it is easy as you mentioned as below.

For generate API (to get the right hash), you need to build the rvv_type_info, predications_type_index and rvv_op_info
from the arglist (aka vec<tree, va_gc>) from hook.

Then we need to construct above parameters from one tree argument. Sorry I not sure if I understand correctly but I failed
to locate somewhere has similar usage.

Could you please help to insight me some best practice about the transformation from tree to above types?

Pan

From: 钟居哲 <juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>>
Sent: Monday, September 11, 2023 9:07 PM
To: Li, Pan2 <pan2.li@intel.com<mailto:pan2.li@intel.com>>
Cc: kito.cheng <kito.cheng@gmail.com<mailto:kito.cheng@gmail.com>>; gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>>; Wang, Yanzhang <yanzhang.wang@intel.com<mailto:yanzhang.wang@intel.com>>
Subject: Re: RE: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic

function_instance
get_read_vl_instance (void)
{
  return function_instance ("read_vl", bases::read_vl, shapes::read_vl,
          none_ops[0], PRED_TYPE_none, &p_none_void_ops);
}

tree
get_read_vl_decl (void)
{
  function_instance instance = get_read_vl_instance ();
  hashval_t hash = instance.hash ();
  registered_function *rfn = function_table->find_with_hash (instance, hash);
  gcc_assert (rfn);
  return rfn->decl;
}

You should reference it. I don't see why it's hard for use to construct instance first, then use that instance hash to get the decl.
________________________________
juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>

From: Li, Pan2<mailto:pan2.li@intel.com>
Date: 2023-09-11 20:26
To: juzhe.zhong<mailto:juzhe.zhong@rivai.ai>
CC: kito.cheng<mailto:kito.cheng@gmail.com>; gcc-patches<mailto:gcc-patches@gcc.gnu.org>; Wang, Yanzhang<mailto:yanzhang.wang@intel.com>
Subject: RE: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic
> No. You must construct instance. 'strcmp' is very ugly.

Strcmp here is defensive code here for early exit if not found (can be removed for correctness), which is not required to find the right declaration.

Pan

From: juzhe.zhong <juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>>
Sent: Monday, September 11, 2023 8:20 PM
To: Li, Pan2 <pan2.li@intel.com<mailto:pan2.li@intel.com>>
Cc: kito.cheng <kito.cheng@gmail.com<mailto:kito.cheng@gmail.com>>; gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>>; Wang, Yanzhang <yanzhang.wang@intel.com<mailto:yanzhang.wang@intel.com>>
Subject: Re: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic

No. You must construct instance. 'strcmp' is very ugly.
---- Replied Message ----
From
Li, Pan2<pan2.li@intel.com><mailto:pan2.li@intel.com>
Date
09/11/2023 20:09
To
juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai><mailto:juzhe.zhong@rivai.ai>,
kito.cheng<kito.cheng@gmail.com><mailto:kito.cheng@gmail.com>
Cc
gcc-patches<gcc-patches@gcc.gnu.org><mailto:gcc-patches@gcc.gnu.org>,
Wang, Yanzhang<yanzhang.wang@intel.com><mailto:yanzhang.wang@intel.com>
Subject
RE: Re: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic
> -    if (overloaded_p && instance.pred == PRED_TYPE_m)
> +    if (overloaded_p)

Thanks for pointing this out, my misunderstanding for policy function result in this change as mistake, will send V2 for this.


> Plz change it into :



Actually, it is not easy to convert to this approach as aarch64 has different implementation of types information.

Like type_suffix_info (aarch64 loop type suffix to get the arglist type in infer_vector_or_tuple_type) etc.

Thus, it is not easy to construct rvv_type_info, predication_type_index and rvv_op_info from arglist, these are required

by function_instance when constructing.



Pan

From: juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai> <juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>>
Sent: Monday, September 11, 2023 5:13 PM
To: kito.cheng <kito.cheng@gmail.com<mailto:kito.cheng@gmail.com>>
Cc: Li, Pan2 <pan2.li@intel.com<mailto:pan2.li@intel.com>>; gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>>; Wang, Yanzhang <yanzhang.wang@intel.com<mailto:yanzhang.wang@intel.com>>
Subject: Re: Re: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic

>> Just make sure it's the right change?
It seem incorrect to me.

More comments (I just reviewed again):


+tree

+function_resolver::lookup ()

+{

+  unsigned int code_limit = vec_safe_length (registered_functions);

+

+  for (unsigned code = get_sub_code () + 1; code < code_limit; code++)

+    {

+      registered_function *rfun = (*registered_functions)[code];

+      function_instance instance = rfun->instance;

+

+      if (strcmp (base_name, instance.base_name) != 0)

+    break;

+

+      if (rfun->overloaded_p)

+    continue;

+

+      unsigned k;

+      const rvv_arg_type_info *args = instance.op_info->args;

+

+      for (k = 0; args[k].base_type != NUM_BASE_TYPES; k++)

+    {

+      if (k >= m_arglist.length ())

+        break;

+

+      if (TYPE_MODE (instance.get_arg_type (k))

+        != TYPE_MODE (TREE_TYPE (m_arglist[k])))

+        break;

+    }

+

+    if (args[k].base_type == NUM_BASE_TYPES)

+      return rfun->decl;

+    }

+

+  return NULL_TREE;

+}



Plz change it into :



/* Silently check whether there is an instance of the function with the

   mode suffix given by MODE and the type suffixes given by TYPE0 and TYPE1.

   Return its function decl if so, otherwise return null.  */

tree

function_resolver::lookup_form (mode_suffix_index mode,

        type_suffix_index type0,

        type_suffix_index type1)

{

  type_suffix_pair types = { type0, type1 };

  function_instance instance (base_name, base, shape, mode, types, pred);

  registered_function *rfn

    = function_table->find_with_hash (instance, instance.hash ());

  return rfn ? rfn->decl : NULL_TREE;

}

________________________________
juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>

From: Kito Cheng<mailto:kito.cheng@gmail.com>
Date: 2023-09-11 17:04
To: juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>
CC: pan2.li<mailto:pan2.li@intel.com>; gcc-patches<mailto:gcc-patches@gcc.gnu.org>; yanzhang.wang<mailto:yanzhang.wang@intel.com>
Subject: Re: [PATCH v1] RISC-V: Implement RESOLVE_OVERLOADED_BUILTIN for RVV intrinsic
> @@ -545,7 +563,7 @@ struct move_def : public build_base
>      /* According to rvv-intrinsic-doc, it does not add "_m" suffix
>         for vop_m C++ overloaded API.  */
> -    if (overloaded_p && instance.pred == PRED_TYPE_m)
> +    if (overloaded_p)

Just make sure it's the right change?

>        return b.finish_name ();
>      b.append_name (predication_suffixes[instance.pred]);
>      return b.finish_name ();