Re: [Mesa-dev] [PATCH 20/23] intel/eu: Rework opcode description tables to allow efficient look-up by either HW or IR opcode.
> >> +nodist_EXTRA_tools_aubinator_SOURCES = dummy.cpp
> >> +
> >> tools_aubinator_CFLAGS = \
> >>$(AM_CFLAGS) \
> >>$(ZLIB_CFLAGS)
> >> @@ -47,6 +49,8 @@ tools_aubinator_LDADD = \
> >> tools_aubinator_error_decode_SOURCES = \
> >>tools/aubinator_error_decode.c
> >>
> >> +nodist_EXTRA_tools_aubinator_error_decode_SOURCES = dummy.cpp
> >> +
> >> tools_aubinator_error_decode_LDADD = \
> >>common/libintel_common.la \
> >>compiler/libintel_compiler.la \
> >
> > This hunk seems to be here by accident.
> >
>
> No, it was required to avoid a link failure in aubinator with the
> autotools build, which is too stupid to realize it needs to use a C++
> linker unless we add these dummy source files.
Ah, got it. Consider adding a comment there about it.
> >> +static const opcode_desc *
> >> +lookup_opcode_desc(gen_device_info *index_devinfo,
> >> + const opcode_desc **index_descs,
> >> + unsigned index_size,
> >> + unsigned opcode_desc::*key,
> >> + const gen_device_info *devinfo,
> >> + unsigned k)
> >> {
> >> - if (opcode >= ARRAY_SIZE(opcode_descs))
> >> - return NULL;
> >> -
> >> - enum gen gen = gen_from_devinfo(devinfo);
> >> - if (opcode_descs[opcode].gens != 0) {
> >> - if ((opcode_descs[opcode].gens & gen) != 0) {
> >> - return _descs[opcode];
> >> - }
> >> - } else if (opcode_descs[opcode].table != NULL) {
> >> - const struct opcode_desc *table = opcode_descs[opcode].table;
> >> - for (unsigned i = 0; i < opcode_descs[opcode].size; i++) {
> >> - if ((table[i].gens & gen) != 0) {
> >> -return [i];
> >> + if (memcmp(index_devinfo, devinfo, sizeof(*devinfo))) {
> >
> > Question: we are comparing a lot of bytes that seem irrelevant for
> > invalidating the table. Is it a win versus what was being done before,
> > i.e. invalidate by gen_device_info?
> >
>
> I'm not 100% clear what you mean by what was being done before since the
> indexing code is new. Are you suggesting to compare that
> gen_from_devinfo(devinfo) == gen_from_devinfo(index_devinfo) instead of
> comparing the devinfo structs directly? Yes, that should work and be
> slightly more efficient, since currently the index data structure only
> depends on the gen number. I've applied the optimization locally.
You read correctly my badly worded comment. Thanks.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 20/23] intel/eu: Rework opcode description tables to allow efficient look-up by either HW or IR opcode.
Caio Marcelo de Oliveira Filho writes:
>> diff --git a/src/intel/Makefile.tools.am b/src/intel/Makefile.tools.am
>> index b00cc8cc2cb..4f2027cdfd6 100644
>> --- a/src/intel/Makefile.tools.am
>> +++ b/src/intel/Makefile.tools.am
>> @@ -27,6 +27,8 @@ tools_aubinator_SOURCES = \
>> tools/aubinator.c \
>> tools/intel_aub.h
>>
>> +nodist_EXTRA_tools_aubinator_SOURCES = dummy.cpp
>> +
>> tools_aubinator_CFLAGS = \
>> $(AM_CFLAGS) \
>> $(ZLIB_CFLAGS)
>> @@ -47,6 +49,8 @@ tools_aubinator_LDADD = \
>> tools_aubinator_error_decode_SOURCES = \
>> tools/aubinator_error_decode.c
>>
>> +nodist_EXTRA_tools_aubinator_error_decode_SOURCES = dummy.cpp
>> +
>> tools_aubinator_error_decode_LDADD = \
>> common/libintel_common.la \
>> compiler/libintel_compiler.la \
>
> This hunk seems to be here by accident.
>
No, it was required to avoid a link failure in aubinator with the
autotools build, which is too stupid to realize it needs to use a C++
linker unless we add these dummy source files.
>
>> +static const opcode_desc *
>> +lookup_opcode_desc(gen_device_info *index_devinfo,
>> + const opcode_desc **index_descs,
>> + unsigned index_size,
>> + unsigned opcode_desc::*key,
>> + const gen_device_info *devinfo,
>> + unsigned k)
>> {
>> - if (opcode >= ARRAY_SIZE(opcode_descs))
>> - return NULL;
>> -
>> - enum gen gen = gen_from_devinfo(devinfo);
>> - if (opcode_descs[opcode].gens != 0) {
>> - if ((opcode_descs[opcode].gens & gen) != 0) {
>> - return _descs[opcode];
>> - }
>> - } else if (opcode_descs[opcode].table != NULL) {
>> - const struct opcode_desc *table = opcode_descs[opcode].table;
>> - for (unsigned i = 0; i < opcode_descs[opcode].size; i++) {
>> - if ((table[i].gens & gen) != 0) {
>> -return [i];
>> + if (memcmp(index_devinfo, devinfo, sizeof(*devinfo))) {
>
> Question: we are comparing a lot of bytes that seem irrelevant for
> invalidating the table. Is it a win versus what was being done before,
> i.e. invalidate by gen_device_info?
>
I'm not 100% clear what you mean by what was being done before since the
indexing code is new. Are you suggesting to compare that
gen_from_devinfo(devinfo) == gen_from_devinfo(index_devinfo) instead of
comparing the devinfo structs directly? Yes, that should work and be
slightly more efficient, since currently the index data structure only
depends on the gen number. I've applied the optimization locally.
>
>
> Thanks,
> Caio
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 20/23] intel/eu: Rework opcode description tables to allow efficient look-up by either HW or IR opcode.
> diff --git a/src/intel/Makefile.tools.am b/src/intel/Makefile.tools.am
> index b00cc8cc2cb..4f2027cdfd6 100644
> --- a/src/intel/Makefile.tools.am
> +++ b/src/intel/Makefile.tools.am
> @@ -27,6 +27,8 @@ tools_aubinator_SOURCES = \
> tools/aubinator.c \
> tools/intel_aub.h
>
> +nodist_EXTRA_tools_aubinator_SOURCES = dummy.cpp
> +
> tools_aubinator_CFLAGS = \
> $(AM_CFLAGS) \
> $(ZLIB_CFLAGS)
> @@ -47,6 +49,8 @@ tools_aubinator_LDADD = \
> tools_aubinator_error_decode_SOURCES = \
> tools/aubinator_error_decode.c
>
> +nodist_EXTRA_tools_aubinator_error_decode_SOURCES = dummy.cpp
> +
> tools_aubinator_error_decode_LDADD = \
> common/libintel_common.la \
> compiler/libintel_compiler.la \
This hunk seems to be here by accident.
> +static const opcode_desc *
> +lookup_opcode_desc(gen_device_info *index_devinfo,
> + const opcode_desc **index_descs,
> + unsigned index_size,
> + unsigned opcode_desc::*key,
> + const gen_device_info *devinfo,
> + unsigned k)
> {
> - if (opcode >= ARRAY_SIZE(opcode_descs))
> - return NULL;
> -
> - enum gen gen = gen_from_devinfo(devinfo);
> - if (opcode_descs[opcode].gens != 0) {
> - if ((opcode_descs[opcode].gens & gen) != 0) {
> - return _descs[opcode];
> - }
> - } else if (opcode_descs[opcode].table != NULL) {
> - const struct opcode_desc *table = opcode_descs[opcode].table;
> - for (unsigned i = 0; i < opcode_descs[opcode].size; i++) {
> - if ((table[i].gens & gen) != 0) {
> -return [i];
> + if (memcmp(index_devinfo, devinfo, sizeof(*devinfo))) {
Question: we are comparing a lot of bytes that seem irrelevant for
invalidating the table. Is it a win versus what was being done before,
i.e. invalidate by gen_device_info?
Thanks,
Caio
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 20/23] intel/eu: Rework opcode description tables to allow efficient look-up by either HW or IR opcode.
This rewrites the current opcode description tables as a more compact
flat data structure. The purpose is to allow efficient constant-time
look-up by either HW or IR opcode, which will allow us to drop the
hard-coded correspondence between HW and IR opcodes -- See the next
commits for the rationale.
brw_eu.c is now built as C++ source so we can take advantage of
pointers to member in order to make the look-up function work
regardless of the opcode_desc member used as look-up key.
---
src/intel/Makefile.sources | 2 +-
src/intel/Makefile.tools.am | 4 +
src/intel/compiler/{brw_eu.c => brw_eu.cpp} | 414 ++--
src/intel/compiler/brw_eu.h | 43 +--
src/intel/compiler/brw_eu_defines.h | 4 +-
src/intel/compiler/meson.build | 2 +-
6 files changed, 170 insertions(+), 299 deletions(-)
rename src/intel/compiler/{brw_eu.c => brw_eu.cpp} (57%)
diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
index f22e727553f..33fdf4a26c7 100644
--- a/src/intel/Makefile.sources
+++ b/src/intel/Makefile.sources
@@ -43,7 +43,7 @@ COMPILER_FILES = \
compiler/brw_disasm.c \
compiler/brw_disasm_info.c \
compiler/brw_disasm_info.h \
- compiler/brw_eu.c \
+ compiler/brw_eu.cpp \
compiler/brw_eu_compact.c \
compiler/brw_eu_defines.h \
compiler/brw_eu_emit.c \
diff --git a/src/intel/Makefile.tools.am b/src/intel/Makefile.tools.am
index b00cc8cc2cb..4f2027cdfd6 100644
--- a/src/intel/Makefile.tools.am
+++ b/src/intel/Makefile.tools.am
@@ -27,6 +27,8 @@ tools_aubinator_SOURCES = \
tools/aubinator.c \
tools/intel_aub.h
+nodist_EXTRA_tools_aubinator_SOURCES = dummy.cpp
+
tools_aubinator_CFLAGS = \
$(AM_CFLAGS) \
$(ZLIB_CFLAGS)
@@ -47,6 +49,8 @@ tools_aubinator_LDADD = \
tools_aubinator_error_decode_SOURCES = \
tools/aubinator_error_decode.c
+nodist_EXTRA_tools_aubinator_error_decode_SOURCES = dummy.cpp
+
tools_aubinator_error_decode_LDADD = \
common/libintel_common.la \
compiler/libintel_compiler.la \
diff --git a/src/intel/compiler/brw_eu.c b/src/intel/compiler/brw_eu.cpp
similarity index 57%
rename from src/intel/compiler/brw_eu.c
rename to src/intel/compiler/brw_eu.cpp
index 80f05240b42..37375005daa 100644
--- a/src/intel/compiler/brw_eu.c
+++ b/src/intel/compiler/brw_eu.cpp
@@ -406,263 +406,87 @@ enum gen {
#define GEN_GE(gen) (~GEN_LT(gen))
#define GEN_LE(gen) (GEN_LT(gen) | (gen))
-static const struct opcode_desc opcode_10_descs[] = {
- { .name = "dim", .nsrc = 1, .ndst = 1, .gens = GEN75 },
- { .name = "smov", .nsrc = 0, .ndst = 0, .gens = GEN_GE(GEN8) },
-};
-
-static const struct opcode_desc opcode_35_descs[] = {
- { .name = "iff", .nsrc = 0, .ndst = 0, .gens = GEN_LE(GEN5) },
- { .name = "brc", .nsrc = 0, .ndst = 0, .gens = GEN_GE(GEN7) },
-};
-
-static const struct opcode_desc opcode_38_descs[] = {
- { .name = "do",.nsrc = 0, .ndst = 0, .gens = GEN_LE(GEN5) },
- { .name = "case", .nsrc = 0, .ndst = 0, .gens = GEN6 },
-};
-
-static const struct opcode_desc opcode_44_descs[] = {
- { .name = "msave", .nsrc = 0, .ndst = 0, .gens = GEN_LE(GEN5) },
- { .name = "call", .nsrc = 0, .ndst = 0, .gens = GEN_GE(GEN6) },
-};
-
-static const struct opcode_desc opcode_45_descs[] = {
- { .name = "mrest", .nsrc = 0, .ndst = 0, .gens = GEN_LE(GEN5) },
- { .name = "ret", .nsrc = 0, .ndst = 0, .gens = GEN_GE(GEN6) },
-};
-
-static const struct opcode_desc opcode_46_descs[] = {
- { .name = "push", .nsrc = 0, .ndst = 0, .gens = GEN_LE(GEN5) },
- { .name = "fork", .nsrc = 0, .ndst = 0, .gens = GEN6 },
- { .name = "goto", .nsrc = 0, .ndst = 0, .gens = GEN_GE(GEN8) },
-};
-
-static const struct opcode_desc opcode_descs[128] = {
- [BRW_OPCODE_ILLEGAL] = {
- .name = "illegal", .nsrc = 0, .ndst = 0, .gens = GEN_ALL,
- },
- [BRW_OPCODE_MOV] = {
- .name = "mov", .nsrc = 1, .ndst = 1, .gens = GEN_ALL,
- },
- [BRW_OPCODE_SEL] = {
- .name = "sel", .nsrc = 2, .ndst = 1, .gens = GEN_ALL,
- },
- [BRW_OPCODE_MOVI] = {
- .name = "movi",.nsrc = 2, .ndst = 1, .gens = GEN_GE(GEN45),
- },
- [BRW_OPCODE_NOT] = {
- .name = "not", .nsrc = 1, .ndst = 1, .gens = GEN_ALL,
- },
- [BRW_OPCODE_AND] = {
- .name = "and", .nsrc = 2, .ndst = 1, .gens = GEN_ALL,
- },
- [BRW_OPCODE_OR] = {
- .name = "or", .nsrc = 2, .ndst = 1, .gens = GEN_ALL,
- },
- [BRW_OPCODE_XOR] = {
- .name = "xor", .nsrc = 2, .ndst = 1, .gens = GEN_ALL,
- },
- [BRW_OPCODE_SHR] = {
- .name = "shr", .nsrc = 2, .ndst = 1, .gens = GEN_ALL,
- },
- [BRW_OPCODE_SHL] = {
- .name = "shl", .nsrc = 2, .ndst = 1, .gens = GEN_ALL,
- },
- [10] = {
- .table = opcode_10_descs, .size = ARRAY_SIZE(opcode_10_descs),
- },
- /* Reserved - 11 */
- [BRW_OPCODE_ASR] = {
- .name = "asr", .nsrc = 2,
