genopinit: Allow more than 256 modes.
Checks
Commit Message
Ok so the consensus seems to rather stay with 32 bits and only
change the shift to 10/20? As MACHINE_MODE_BITSIZE is already
16 we would need an additional check independent of that.
Wouldn't that also be a bit confusing?
Attached is a "v2" with unsigned long long changed to
uint64_t and checking for NUM_OPTABS <= 0xffffffff.
Regards
Robin
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 uint64_t 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 runtimes are
actually unchanged (on 64-bit architectures that is).
gcc/ChangeLog:
* genopinit.cc (pattern_cmp): Use if/else for comparison instead
of subtraction.
(main): Change to uint64_t.
* gensupport.cc (find_optab): Ditto.
* gensupport.h (struct optab_pattern): Ditto.
* optabs-query.h (optab_handler): Ditto.
(convert_optab_handler): Ditto.
---
gcc/genopinit.cc | 21 +++++++++++++--------
gcc/gensupport.cc | 2 +-
gcc/gensupport.h | 2 +-
gcc/optabs-query.h | 4 ++--
4 files changed, 17 insertions(+), 12 deletions(-)
Comments
For example:
https://godbolt.org/z/1d6v5WKhY
Clang can vectorize but GCC failed even with -ffast-math.
So I think conversions should be well checked again to make sure every variant can vectorize.
Thanks.
juzhe.zhong@rivai.ai
From: Robin Dapp
Date: 2023-07-11 21:31
To: Robin Dapp via Gcc-patches; jeffreyalaw; juzhe.zhong@rivai.ai; richard.sandiford; Richard Biener
CC: rdapp.gcc
Subject: [PATCH] genopinit: Allow more than 256 modes.
Ok so the consensus seems to rather stay with 32 bits and only
change the shift to 10/20? As MACHINE_MODE_BITSIZE is already
16 we would need an additional check independent of that.
Wouldn't that also be a bit confusing?
Attached is a "v2" with unsigned long long changed to
uint64_t and checking for NUM_OPTABS <= 0xffffffff.
Regards
Robin
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 uint64_t 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 runtimes are
actually unchanged (on 64-bit architectures that is).
gcc/ChangeLog:
* genopinit.cc (pattern_cmp): Use if/else for comparison instead
of subtraction.
(main): Change to uint64_t.
* gensupport.cc (find_optab): Ditto.
* gensupport.h (struct optab_pattern): Ditto.
* optabs-query.h (optab_handler): Ditto.
(convert_optab_handler): Ditto.
---
gcc/genopinit.cc | 21 +++++++++++++--------
gcc/gensupport.cc | 2 +-
gcc/gensupport.h | 2 +-
gcc/optabs-query.h | 4 ++--
4 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/gcc/genopinit.cc b/gcc/genopinit.cc
index 6bd8858a1d9..05316ccb409 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
@@ -182,7 +187,7 @@ main (int argc, const char **argv)
progname = "genopinit";
- if (NUM_OPTABS > 0xffff
+ if (NUM_OPTABS > 0xffffffff
|| MAX_MACHINE_MODE >= ((1 << MACHINE_MODE_BITSIZE) - 1))
fatal ("genopinit range assumptions inv
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 uint64_t and allows up to 16 bits for a mode as well
as 32 bits for an optab.alid");
@@ -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 (uint64_t);\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"
+ " uint64_t 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 (uint64_t 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 (uint64_t 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"
+ " uint64_t scode = ((uint64_t)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..68df90fce58 100644
--- a/gcc/gensupport.cc
+++ b/gcc/gensupport.cc
@@ -3806,7 +3806,7 @@ 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 = ((uint64_t) p->op << 32) | (p->m2 << 16) | p->m1;
return true;
}
}
diff --git a/gcc/gensupport.h b/gcc/gensupport.h
index 7925e22ed41..247b375f965 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;
+ uint64_t sort_num;
};
extern rtx add_implicit_parallel (rtvec);
diff --git a/gcc/optabs-query.h b/gcc/optabs-query.h
index 043e9791bc1..c6b38da5b26 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;
+ uint64_t scode = ((uint64_t)op << 32) | mode;
gcc_assert (op > LAST_CONV_OPTAB);
return raw_optab_handler (scode);
}
@@ -50,7 +50,7 @@ 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;
+ uint64_t scode = ((uint64_t) op << 32) | (from_mode << 16) | to_mode;
gcc_assert (convert_optab_p (op));
return raw_optab_handler (scode);
}
--
2.41.0
Sorry for sending incorrect email.
Forget about this:).....
juzhe.zhong@rivai.ai
From: 钟居哲
Date: 2023-07-11 21:55
To: rdapp.gcc; gcc-patches; Jeff Law; richard.sandiford; rguenther
CC: rdapp.gcc
Subject: Re: [PATCH] genopinit: Allow more than 256 modes.
For example:
https://godbolt.org/z/1d6v5WKhY
Clang can vectorize but GCC failed even with -ffast-math.
So I think conversions should be well checked again to make sure every variant can vectorize.
Thanks.
juzhe.zhong@rivai.ai
From: Robin Dapp
Date: 2023-07-11 21:31
To: Robin Dapp via Gcc-patches; jeffreyalaw; juzhe.zhong@rivai.ai; richard.sandiford; Richard Biener
CC: rdapp.gcc
Subject: [PATCH] genopinit: Allow more than 256 modes.
Ok so the consensus seems to rather stay with 32 bits and only
change the shift to 10/20? As MACHINE_MODE_BITSIZE is already
16 we would need an additional check independent of that.
Wouldn't that also be a bit confusing?
Attached is a "v2" with unsigned long long changed to
uint64_t and checking for NUM_OPTABS <= 0xffffffff.
Regards
Robin
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 uint64_t 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 runtimes are
actually unchanged (on 64-bit architectures that is).
gcc/ChangeLog:
* genopinit.cc (pattern_cmp): Use if/else for comparison instead
of subtraction.
(main): Change to uint64_t.
* gensupport.cc (find_optab): Ditto.
* gensupport.h (struct optab_pattern): Ditto.
* optabs-query.h (optab_handler): Ditto.
(convert_optab_handler): Ditto.
---
gcc/genopinit.cc | 21 +++++++++++++--------
gcc/gensupport.cc | 2 +-
gcc/gensupport.h | 2 +-
gcc/optabs-query.h | 4 ++--
4 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/gcc/genopinit.cc b/gcc/genopinit.cc
index 6bd8858a1d9..05316ccb409 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
@@ -182,7 +187,7 @@ main (int argc, const char **argv)
progname = "genopinit";
- if (NUM_OPTABS > 0xffff
+ if (NUM_OPTABS > 0xffffffff
|| MAX_MACHINE_MODE >= ((1 << MACHINE_MODE_BITSIZE) - 1))
fatal ("genopinit range assumptions inv
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 uint64_t and allows up to 16 bits for a mode as well
as 32 bits for an optab.alid");
@@ -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 (uint64_t);\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"
+ " uint64_t 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 (uint64_t 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 (uint64_t 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"
+ " uint64_t scode = ((uint64_t)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..68df90fce58 100644
--- a/gcc/gensupport.cc
+++ b/gcc/gensupport.cc
@@ -3806,7 +3806,7 @@ 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 = ((uint64_t) p->op << 32) | (p->m2 << 16) | p->m1;
return true;
}
}
diff --git a/gcc/gensupport.h b/gcc/gensupport.h
index 7925e22ed41..247b375f965 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;
+ uint64_t sort_num;
};
extern rtx add_implicit_parallel (rtvec);
diff --git a/gcc/optabs-query.h b/gcc/optabs-query.h
index 043e9791bc1..c6b38da5b26 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;
+ uint64_t scode = ((uint64_t)op << 32) | mode;
gcc_assert (op > LAST_CONV_OPTAB);
return raw_optab_handler (scode);
}
@@ -50,7 +50,7 @@ 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;
+ uint64_t scode = ((uint64_t) op << 32) | (from_mode << 16) | to_mode;
gcc_assert (convert_optab_p (op));
return raw_optab_handler (scode);
}
--
2.41.0
Robin Dapp <rdapp.gcc@gmail.com> writes:
> Ok so the consensus seems to rather stay with 32 bits and only
> change the shift to 10/20?
Yeah. The check would then be:
if (NUM_OPTABS > 0xfff || NUM_MACHINE_MODES > 0x3ff)
fatal ("genopinit range assumptions invalid");
> As MACHINE_MODE_BITSIZE is already
> 16 we would need an additional check independent of that.
> Wouldn't that also be a bit confusing?
MACHINE_MODE_BITSIZE isn't really relevant to genopinit.cc.
If we wanted to make it relevant, we ought to express everything
that depends on the mode size in terms of MACHINE_MODE_BITSIZE,
so that the shifts would be << MACHINE_MODE_BITSIZE and
<< (2 * MACHINE_MODE_BITSIZE). The NUM_OPTABS condition would
need to be expressed in terms of MACHINE_MODE_BITSIZE too.
But that seems overkill. The reason for going straight from 8 to 16 for
the bitfields was to keep the accesses fast. But that isn't an issue
for these shifts, since the end result is just ORed into a 32-bit blob.
And 32 bits are still plenty, even with the new RISC-V modes.
Thanks,
Richard
@@ -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
@@ -182,7 +187,7 @@ main (int argc, const char **argv)
progname = "genopinit";
- if (NUM_OPTABS > 0xffff
+ if (NUM_OPTABS > 0xffffffff
|| MAX_MACHINE_MODE >= ((1 << MACHINE_MODE_BITSIZE) - 1))
fatal ("genopinit range assumptions inv
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 uint64_t and allows up to 16 bits for a mode as well
as 32 bits for an optab.alid");
@@ -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 (uint64_t);\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"
+ " uint64_t 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 (uint64_t 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 (uint64_t 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"
+ " uint64_t scode = ((uint64_t)op << 32) | m;\n"
" int i = lookup_handler (scode);\n"
" if (i >= 0)\n"
" {\n"
@@ -3806,7 +3806,7 @@ 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 = ((uint64_t) p->op << 32) | (p->m2 << 16) | p->m1;
return true;
}
}
@@ -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;
+ uint64_t sort_num;
};
extern rtx add_implicit_parallel (rtvec);
@@ -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;
+ uint64_t scode = ((uint64_t)op << 32) | mode;
gcc_assert (op > LAST_CONV_OPTAB);
return raw_optab_handler (scode);
}
@@ -50,7 +50,7 @@ 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;
+ uint64_t scode = ((uint64_t) op << 32) | (from_mode << 16) | to_mode;
gcc_assert (convert_optab_p (op));
return raw_optab_handler (scode);
}