genopinit: Allow more than 256 modes.

Hi,

upcoming changes for RISC-V will have us exceed 256 modes or 8 bits. The
helper functions in gen* rely on the opcode as well as two modes fitting
into an unsigned int (a signed int even if we consider the qsort default
comparison function).  This patch changes the type of the index/hash
from unsigned int to unsigned long long and allows up to 16 bits for a
mode as well as 32 bits for an optab.

Despite fearing worse, bootstrap, build and test suite run times on
x86, aarch64, rv64 and power10 are actually unchanged (I didn't check
32-bit architectures but would expect similar results).

Regards
 Robin

gcc/ChangeLog:

	* genopinit.cc (pattern_cmp): Use if/else for comparison instead
	of subtraction.
	(main): Change to unsigned long long.
	* gensupport.cc (find_optab): Ditto.
	* gensupport.h (struct optab_pattern): Ditto.
	* optabs-query.h (optab_handler): Ditto.
	(convert_optab_handler): Ditto.
---
 gcc/genopinit.cc   | 19 ++++++++++++-------
 gcc/gensupport.cc  |  3 ++-
 gcc/gensupport.h   |  2 +-
 gcc/optabs-query.h |  5 +++--
 4 files changed, 18 insertions(+), 11 deletions(-)

Thanks for fixing it.
CC Richards to see whether it is appropriate.



juzhe.zhong@rivai.ai
 
From: Robin Dapp
Date: 2023-07-11 19:51
To: gcc-patches
CC: rdapp.gcc; jeffreyalaw; juzhe.zhong@rivai.ai
Subject: [PATCH] genopinit: Allow more than 256 modes.
Hi,
 
upcoming changes for RISC-V will have us exceed 256 modes or 8 bits. The
helper functions in gen* rely on the opcode as well as two modes fitting
into an unsigned int (a signed int even if we consider the qsort default
comparison function).  This patch changes the type of the index/hash
from unsigned int to unsigned long long and allows up to 16 bits for a
mode as well as 32 bits for an optab.
 
Despite fearing worse, bootstrap, build and test suite run times on
x86, aarch64, rv64 and power10 are actually unchanged (I didn't check
32-bit architectures but would expect similar results).
 
Regards
Robin
 
gcc/ChangeLog:
 
* genopinit.cc (pattern_cmp): Use if/else for comparison instead
of subtraction.
(main): Change to unsigned long long.
* gensupport.cc (find_optab): Ditto.
* gensupport.h (struct optab_pattern): Ditto.
* optabs-query.h (optab_handler): Ditto.
(convert_optab_handler): Ditto.
---
gcc/genopinit.cc   | 19 ++++++++++++-------
gcc/gensupport.cc  |  3 ++-
gcc/gensupport.h   |  2 +-
gcc/optabs-query.h |  5 +++--
4 files changed, 18 insertions(+), 11 deletions(-)
 
diff --git a/gcc/genopinit.cc b/gcc/genopinit.cc
index 6bd8858a1d9..58c1bf7cba8 100644
--- a/gcc/genopinit.cc
+++ b/gcc/genopinit.cc
@@ -51,7 +51,12 @@ pattern_cmp (const void *va, const void *vb)
{
   const optab_pattern *a = (const optab_pattern *)va;
   const optab_pattern *b = (const optab_pattern *)vb;
-  return a->sort_num - b->sort_num;
+  if (a->sort_num > b->sort_num)
+    return 1;
+  else if (a->sort_num < b->sort_num)
+    return -1;
+  else
+    return 0;
}
static int
@@ -306,7 +311,7 @@ main (int argc, const char **argv)
   "extern const struct optab_libcall_d normlib_def[NUM_NORMLIB_OPTABS];\n"
   "\n"
   "/* Returns the active icode for the given (encoded) optab.  */\n"
-    "extern enum insn_code raw_optab_handler (unsigned);\n"
+    "extern enum insn_code raw_optab_handler (unsigned long long);\n"
   "extern bool swap_optab_enable (optab, machine_mode, bool);\n"
   "\n"
   "/* Target-dependent globals.  */\n"
@@ -358,14 +363,14 @@ main (int argc, const char **argv)
   "#include \"optabs.h\"\n"
   "\n"
   "struct optab_pat {\n"
-    "  unsigned scode;\n"
+    "  unsigned long long scode;\n"
   "  enum insn_code icode;\n"
   "};\n\n");
   fprintf (s_file,
   "static const struct optab_pat pats[NUM_OPTAB_PATTERNS] = {\n");
   for (i = 0; patterns.iterate (i, &p); ++i)
-    fprintf (s_file, "  { %#08x, CODE_FOR_%s },\n", p->sort_num, p->name);
+    fprintf (s_file, "  { %#08llx, CODE_FOR_%s },\n", p->sort_num, p->name);
   fprintf (s_file, "};\n\n");
   fprintf (s_file, "void\ninit_all_optabs (struct target_optabs *optabs)\n{\n");
@@ -410,7 +415,7 @@ main (int argc, const char **argv)
      the hash entries, which complicates the pat_enable array.  */
   fprintf (s_file,
   "static int\n"
-    "lookup_handler (unsigned scode)\n"
+    "lookup_handler (unsigned long long scode)\n"
   "{\n"
   "  int l = 0, h = ARRAY_SIZE (pats), m;\n"
   "  while (h > l)\n"
@@ -428,7 +433,7 @@ main (int argc, const char **argv)
   fprintf (s_file,
   "enum insn_code\n"
-    "raw_optab_handler (unsigned scode)\n"
+    "raw_optab_handler (unsigned long long scode)\n"
   "{\n"
   "  int i = lookup_handler (scode);\n"
   "  return (i >= 0 && this_fn_optabs->pat_enable[i]\n"
@@ -439,7 +444,7 @@ main (int argc, const char **argv)
   "bool\n"
   "swap_optab_enable (optab op, machine_mode m, bool set)\n"
   "{\n"
-    "  unsigned scode = (op << 16) | m;\n"
+    "  unsigned long long scode = ((unsigned long long)op << 32) | m;\n"
   "  int i = lookup_handler (scode);\n"
   "  if (i >= 0)\n"
   "    {\n"
diff --git a/gcc/gensupport.cc b/gcc/gensupport.cc
index e39e6dacce2..3fe7428372d 100644
--- a/gcc/gensupport.cc
+++ b/gcc/gensupport.cc
@@ -3806,7 +3806,8 @@ find_optab (optab_pattern *p, const char *name)
{
  p->name = name;
  p->op = optabs[pindex].op;
-   p->sort_num = (p->op << 16) | (p->m2 << 8) | p->m1;
+   p->sort_num
+     = ((unsigned long long) p->op << 32) | (p->m2 << 16) | p->m1;
  return true;
}
     }
diff --git a/gcc/gensupport.h b/gcc/gensupport.h
index 7925e22ed41..9f70e2310e2 100644
--- a/gcc/gensupport.h
+++ b/gcc/gensupport.h
@@ -123,7 +123,7 @@ struct optab_pattern
   /* An index that provides a lexicographical sort of (OP, M2, M1).
      Used by genopinit.cc.  */
-  unsigned int sort_num;
+  unsigned long long sort_num;
};
extern rtx add_implicit_parallel (rtvec);
diff --git a/gcc/optabs-query.h b/gcc/optabs-query.h
index 043e9791bc1..5a1d2f75470 100644
--- a/gcc/optabs-query.h
+++ b/gcc/optabs-query.h
@@ -37,7 +37,7 @@ convert_optab_p (optab op)
inline enum insn_code
optab_handler (optab op, machine_mode mode)
{
-  unsigned scode = (op << 16) | mode;
+  unsigned long long scode = ((unsigned long long)op << 32) | mode;
   gcc_assert (op > LAST_CONV_OPTAB);
   return raw_optab_handler (scode);
}
@@ -50,7 +50,8 @@ inline enum insn_code
convert_optab_handler (convert_optab op, machine_mode to_mode,
       machine_mode from_mode)
{
-  unsigned scode = (op << 16) | (from_mode << 8) | to_mode;
+  unsigned long long scode
+    = ((unsigned long long) op << 32) | (from_mode << 16) | to_mode;
   gcc_assert (convert_optab_p (op));
   return raw_optab_handler (scode);
}

Robin Dapp via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Hi,
>
> upcoming changes for RISC-V will have us exceed 256 modes or 8 bits. The
> helper functions in gen* rely on the opcode as well as two modes fitting
> into an unsigned int (a signed int even if we consider the qsort default
> comparison function).  This patch changes the type of the index/hash
> from unsigned int to unsigned long long and allows up to 16 bits for a
> mode as well as 32 bits for an optab.
>
> Despite fearing worse, bootstrap, build and test suite run times on
> x86, aarch64, rv64 and power10 are actually unchanged (I didn't check
> 32-bit architectures but would expect similar results).

I think for now we should just bump the mode shift to 10 and assert
(statically) that MAX_MACHINE_MODE < 1024.

Thanks,
Richard

> Regards
>  Robin
>
> gcc/ChangeLog:
>
> 	* genopinit.cc (pattern_cmp): Use if/else for comparison instead
> 	of subtraction.
> 	(main): Change to unsigned long long.
> 	* gensupport.cc (find_optab): Ditto.
> 	* gensupport.h (struct optab_pattern): Ditto.
> 	* optabs-query.h (optab_handler): Ditto.
> 	(convert_optab_handler): Ditto.
> ---
>  gcc/genopinit.cc   | 19 ++++++++++++-------
>  gcc/gensupport.cc  |  3 ++-
>  gcc/gensupport.h   |  2 +-
>  gcc/optabs-query.h |  5 +++--
>  4 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/gcc/genopinit.cc b/gcc/genopinit.cc
> index 6bd8858a1d9..58c1bf7cba8 100644
> --- a/gcc/genopinit.cc
> +++ b/gcc/genopinit.cc
> @@ -51,7 +51,12 @@ pattern_cmp (const void *va, const void *vb)
>  {
>    const optab_pattern *a = (const optab_pattern *)va;
>    const optab_pattern *b = (const optab_pattern *)vb;
> -  return a->sort_num - b->sort_num;
> +  if (a->sort_num > b->sort_num)
> +    return 1;
> +  else if (a->sort_num < b->sort_num)
> +    return -1;
> +  else
> +    return 0;
>  }
>  
>  static int
> @@ -306,7 +311,7 @@ main (int argc, const char **argv)
>  	   "extern const struct optab_libcall_d normlib_def[NUM_NORMLIB_OPTABS];\n"
>  	   "\n"
>  	   "/* Returns the active icode for the given (encoded) optab.  */\n"
> -	   "extern enum insn_code raw_optab_handler (unsigned);\n"
> +	   "extern enum insn_code raw_optab_handler (unsigned long long);\n"
>  	   "extern bool swap_optab_enable (optab, machine_mode, bool);\n"
>  	   "\n"
>  	   "/* Target-dependent globals.  */\n"
> @@ -358,14 +363,14 @@ main (int argc, const char **argv)
>  	   "#include \"optabs.h\"\n"
>  	   "\n"
>  	   "struct optab_pat {\n"
> -	   "  unsigned scode;\n"
> +	   "  unsigned long long scode;\n"
>  	   "  enum insn_code icode;\n"
>  	   "};\n\n");
>  
>    fprintf (s_file,
>  	   "static const struct optab_pat pats[NUM_OPTAB_PATTERNS] = {\n");
>    for (i = 0; patterns.iterate (i, &p); ++i)
> -    fprintf (s_file, "  { %#08x, CODE_FOR_%s },\n", p->sort_num, p->name);
> +    fprintf (s_file, "  { %#08llx, CODE_FOR_%s },\n", p->sort_num, p->name);
>    fprintf (s_file, "};\n\n");
>  
>    fprintf (s_file, "void\ninit_all_optabs (struct target_optabs *optabs)\n{\n");
> @@ -410,7 +415,7 @@ main (int argc, const char **argv)
>       the hash entries, which complicates the pat_enable array.  */
>    fprintf (s_file,
>  	   "static int\n"
> -	   "lookup_handler (unsigned scode)\n"
> +	   "lookup_handler (unsigned long long scode)\n"
>  	   "{\n"
>  	   "  int l = 0, h = ARRAY_SIZE (pats), m;\n"
>  	   "  while (h > l)\n"
> @@ -428,7 +433,7 @@ main (int argc, const char **argv)
>  
>    fprintf (s_file,
>  	   "enum insn_code\n"
> -	   "raw_optab_handler (unsigned scode)\n"
> +	   "raw_optab_handler (unsigned long long scode)\n"
>  	   "{\n"
>  	   "  int i = lookup_handler (scode);\n"
>  	   "  return (i >= 0 && this_fn_optabs->pat_enable[i]\n"
> @@ -439,7 +444,7 @@ main (int argc, const char **argv)
>  	   "bool\n"
>  	   "swap_optab_enable (optab op, machine_mode m, bool set)\n"
>  	   "{\n"
> -	   "  unsigned scode = (op << 16) | m;\n"
> +	   "  unsigned long long scode = ((unsigned long long)op << 32) | m;\n"
>  	   "  int i = lookup_handler (scode);\n"
>  	   "  if (i >= 0)\n"
>  	   "    {\n"
> diff --git a/gcc/gensupport.cc b/gcc/gensupport.cc
> index e39e6dacce2..3fe7428372d 100644
> --- a/gcc/gensupport.cc
> +++ b/gcc/gensupport.cc
> @@ -3806,7 +3806,8 @@ find_optab (optab_pattern *p, const char *name)
>  	{
>  	  p->name = name;
>  	  p->op = optabs[pindex].op;
> -	  p->sort_num = (p->op << 16) | (p->m2 << 8) | p->m1;
> +	  p->sort_num
> +	    = ((unsigned long long) p->op << 32) | (p->m2 << 16) | p->m1;
>  	  return true;
>  	}
>      }
> diff --git a/gcc/gensupport.h b/gcc/gensupport.h
> index 7925e22ed41..9f70e2310e2 100644
> --- a/gcc/gensupport.h
> +++ b/gcc/gensupport.h
> @@ -123,7 +123,7 @@ struct optab_pattern
>  
>    /* An index that provides a lexicographical sort of (OP, M2, M1).
>       Used by genopinit.cc.  */
> -  unsigned int sort_num;
> +  unsigned long long sort_num;
>  };
>  
>  extern rtx add_implicit_parallel (rtvec);
> diff --git a/gcc/optabs-query.h b/gcc/optabs-query.h
> index 043e9791bc1..5a1d2f75470 100644
> --- a/gcc/optabs-query.h
> +++ b/gcc/optabs-query.h
> @@ -37,7 +37,7 @@ convert_optab_p (optab op)
>  inline enum insn_code
>  optab_handler (optab op, machine_mode mode)
>  {
> -  unsigned scode = (op << 16) | mode;
> +  unsigned long long scode = ((unsigned long long)op << 32) | mode;
>    gcc_assert (op > LAST_CONV_OPTAB);
>    return raw_optab_handler (scode);
>  }
> @@ -50,7 +50,8 @@ inline enum insn_code
>  convert_optab_handler (convert_optab op, machine_mode to_mode,
>  		       machine_mode from_mode)
>  {
> -  unsigned scode = (op << 16) | (from_mode << 8) | to_mode;
> +  unsigned long long scode
> +    = ((unsigned long long) op << 32) | (from_mode << 16) | to_mode;
>    gcc_assert (convert_optab_p (op));
>    return raw_optab_handler (scode);
>  }

Richard Sandiford <richard.sandiford@arm.com> writes:
> Robin Dapp via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> Hi,
>>
>> upcoming changes for RISC-V will have us exceed 256 modes or 8 bits. The
>> helper functions in gen* rely on the opcode as well as two modes fitting
>> into an unsigned int (a signed int even if we consider the qsort default
>> comparison function).  This patch changes the type of the index/hash
>> from unsigned int to unsigned long long and allows up to 16 bits for a
>> mode as well as 32 bits for an optab.
>>
>> Despite fearing worse, bootstrap, build and test suite run times on
>> x86, aarch64, rv64 and power10 are actually unchanged (I didn't check
>> 32-bit architectures but would expect similar results).
>
> I think for now we should just bump the mode shift to 10 and assert
> (statically) that MAX_MACHINE_MODE < 1024.

Sorry, just remembered that we already have:

  if (NUM_OPTABS > 0xffff
    || MAX_MACHINE_MODE >= ((1 << MACHINE_MODE_BITSIZE) - 1))
    fatal ("genopinit range assumptions invalid");

so it would be a case of changing those instead.

Thanks,
Richard

>   if (NUM_OPTABS > 0xffff
>     || MAX_MACHINE_MODE >= ((1 << MACHINE_MODE_BITSIZE) - 1))
>     fatal ("genopinit range assumptions invalid");
> 
> so it would be a case of changing those instead.

Thanks, right at the beginning of the file and I didn't see it ;)
MACHINE_MODE_BITSIZE is already 16, Pan changed that for one of
the previous patches.  Should we bump the NUM_OPTABS to
0xffffffff now, i.e. in this patch or only when the need arises?

Regards
 Robin

On Tue, 11 Jul 2023, ??? wrote:

> Thanks for fixing it.
> CC Richards to see whether it is appropriate.

I agree with Richard S., but generally please avoid
'long long' and use stdint types when you need specific
precision.

Richard.

> 
> 
> juzhe.zhong@rivai.ai
>  
> From: Robin Dapp
> Date: 2023-07-11 19:51
> To: gcc-patches
> CC: rdapp.gcc; jeffreyalaw; juzhe.zhong@rivai.ai
> Subject: [PATCH] genopinit: Allow more than 256 modes.
> Hi,
>  
> upcoming changes for RISC-V will have us exceed 256 modes or 8 bits. The
> helper functions in gen* rely on the opcode as well as two modes fitting
> into an unsigned int (a signed int even if we consider the qsort default
> comparison function).  This patch changes the type of the index/hash
> from unsigned int to unsigned long long and allows up to 16 bits for a
> mode as well as 32 bits for an optab.
>  
> Despite fearing worse, bootstrap, build and test suite run times on
> x86, aarch64, rv64 and power10 are actually unchanged (I didn't check
> 32-bit architectures but would expect similar results).
>  
> Regards
> Robin
>  
> gcc/ChangeLog:
>  
> * genopinit.cc (pattern_cmp): Use if/else for comparison instead
> of subtraction.
> (main): Change to unsigned long long.
> * gensupport.cc (find_optab): Ditto.
> * gensupport.h (struct optab_pattern): Ditto.
> * optabs-query.h (optab_handler): Ditto.
> (convert_optab_handler): Ditto.
> ---
> gcc/genopinit.cc   | 19 ++++++++++++-------
> gcc/gensupport.cc  |  3 ++-
> gcc/gensupport.h   |  2 +-
> gcc/optabs-query.h |  5 +++--
> 4 files changed, 18 insertions(+), 11 deletions(-)
>  
> diff --git a/gcc/genopinit.cc b/gcc/genopinit.cc
> index 6bd8858a1d9..58c1bf7cba8 100644
> --- a/gcc/genopinit.cc
> +++ b/gcc/genopinit.cc
> @@ -51,7 +51,12 @@ pattern_cmp (const void *va, const void *vb)
> {
>    const optab_pattern *a = (const optab_pattern *)va;
>    const optab_pattern *b = (const optab_pattern *)vb;
> -  return a->sort_num - b->sort_num;
> +  if (a->sort_num > b->sort_num)
> +    return 1;
> +  else if (a->sort_num < b->sort_num)
> +    return -1;
> +  else
> +    return 0;
> }
> static int
> @@ -306,7 +311,7 @@ main (int argc, const char **argv)
>    "extern const struct optab_libcall_d normlib_def[NUM_NORMLIB_OPTABS];\n"
>    "\n"
>    "/* Returns the active icode for the given (encoded) optab.  */\n"
> -    "extern enum insn_code raw_optab_handler (unsigned);\n"
> +    "extern enum insn_code raw_optab_handler (unsigned long long);\n"
>    "extern bool swap_optab_enable (optab, machine_mode, bool);\n"
>    "\n"
>    "/* Target-dependent globals.  */\n"
> @@ -358,14 +363,14 @@ main (int argc, const char **argv)
>    "#include \"optabs.h\"\n"
>    "\n"
>    "struct optab_pat {\n"
> -    "  unsigned scode;\n"
> +    "  unsigned long long scode;\n"
>    "  enum insn_code icode;\n"
>    "};\n\n");
>    fprintf (s_file,
>    "static const struct optab_pat pats[NUM_OPTAB_PATTERNS] = {\n");
>    for (i = 0; patterns.iterate (i, &p); ++i)
> -    fprintf (s_file, "  { %#08x, CODE_FOR_%s },\n", p->sort_num, p->name);
> +    fprintf (s_file, "  { %#08llx, CODE_FOR_%s },\n", p->sort_num, p->name);
>    fprintf (s_file, "};\n\n");
>    fprintf (s_file, "void\ninit_all_optabs (struct target_optabs *optabs)\n{\n");
> @@ -410,7 +415,7 @@ main (int argc, const char **argv)
>       the hash entries, which complicates the pat_enable array.  */
>    fprintf (s_file,
>    "static int\n"
> -    "lookup_handler (unsigned scode)\n"
> +    "lookup_handler (unsigned long long scode)\n"
>    "{\n"
>    "  int l = 0, h = ARRAY_SIZE (pats), m;\n"
>    "  while (h > l)\n"
> @@ -428,7 +433,7 @@ main (int argc, const char **argv)
>    fprintf (s_file,
>    "enum insn_code\n"
> -    "raw_optab_handler (unsigned scode)\n"
> +    "raw_optab_handler (unsigned long long scode)\n"
>    "{\n"
>    "  int i = lookup_handler (scode);\n"
>    "  return (i >= 0 && this_fn_optabs->pat_enable[i]\n"
> @@ -439,7 +444,7 @@ main (int argc, const char **argv)
>    "bool\n"
>    "swap_optab_enable (optab op, machine_mode m, bool set)\n"
>    "{\n"
> -    "  unsigned scode = (op << 16) | m;\n"
> +    "  unsigned long long scode = ((unsigned long long)op << 32) | m;\n"
>    "  int i = lookup_handler (scode);\n"
>    "  if (i >= 0)\n"
>    "    {\n"
> diff --git a/gcc/gensupport.cc b/gcc/gensupport.cc
> index e39e6dacce2..3fe7428372d 100644
> --- a/gcc/gensupport.cc
> +++ b/gcc/gensupport.cc
> @@ -3806,7 +3806,8 @@ find_optab (optab_pattern *p, const char *name)
> {
>   p->name = name;
>   p->op = optabs[pindex].op;
> -   p->sort_num = (p->op << 16) | (p->m2 << 8) | p->m1;
> +   p->sort_num
> +     = ((unsigned long long) p->op << 32) | (p->m2 << 16) | p->m1;
>   return true;
> }
>      }
> diff --git a/gcc/gensupport.h b/gcc/gensupport.h
> index 7925e22ed41..9f70e2310e2 100644
> --- a/gcc/gensupport.h
> +++ b/gcc/gensupport.h
> @@ -123,7 +123,7 @@ struct optab_pattern
>    /* An index that provides a lexicographical sort of (OP, M2, M1).
>       Used by genopinit.cc.  */
> -  unsigned int sort_num;
> +  unsigned long long sort_num;
> };
> extern rtx add_implicit_parallel (rtvec);
> diff --git a/gcc/optabs-query.h b/gcc/optabs-query.h
> index 043e9791bc1..5a1d2f75470 100644
> --- a/gcc/optabs-query.h
> +++ b/gcc/optabs-query.h
> @@ -37,7 +37,7 @@ convert_optab_p (optab op)
> inline enum insn_code
> optab_handler (optab op, machine_mode mode)
> {
> -  unsigned scode = (op << 16) | mode;
> +  unsigned long long scode = ((unsigned long long)op << 32) | mode;
>    gcc_assert (op > LAST_CONV_OPTAB);
>    return raw_optab_handler (scode);
> }
> @@ -50,7 +50,8 @@ inline enum insn_code
> convert_optab_handler (convert_optab op, machine_mode to_mode,
>        machine_mode from_mode)
> {
> -  unsigned scode = (op << 16) | (from_mode << 8) | to_mode;
> +  unsigned long long scode
> +    = ((unsigned long long) op << 32) | (from_mode << 16) | to_mode;
>    gcc_assert (convert_optab_p (op));
>    return raw_optab_handler (scode);
> }
>