[Mesa-dev] [PATCH 8/8] i965: Add disassembler support for remaining opcodes.
Francisco Jerez
currojerez at riseup.net
Sun May 1 20:51:24 UTC 2016
Matt Turner <mattst88 at gmail.com> writes:
> For opcodes that changed meaning on different generations, we store a
> pointer to a secondary table and the table's size in a tagged union in
> place of the mnemonic and number of sources.
> ---
> src/mesa/drivers/dri/i965/brw_eu.c | 85 +++++++++++++++++++++++++++++++-------
> src/mesa/drivers/dri/i965/brw_eu.h | 12 +++++-
> 2 files changed, 81 insertions(+), 16 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.c b/src/mesa/drivers/dri/i965/brw_eu.c
> index 9dce5cc..4dacac7 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu.c
> @@ -355,6 +355,37 @@ enum gen {
> #define GEN_GE(gen) (~((gen) - 1) | gen)
> #define GEN_LE(gen) (((gen) - 1) | gen)
>
> +static const struct opcode_desc opcode_10_descs[] = {
> + { .name = "dim", .nsrc = 0, .ndst = 0, .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,
> @@ -386,7 +417,9 @@ static const struct opcode_desc opcode_descs[128] = {
> [BRW_OPCODE_SHL] = {
> .name = "shl", .nsrc = 2, .ndst = 1, .gens = GEN_ALL,
> },
> - /* BRW_OPCODE_DIM / BRW_OPCODE_SMOV */
> + [10] = {
> + .table = opcode_10_descs, .size = ARRAY_SIZE(opcode_10_descs),
> + },
> /* Reserved - 11 */
> [BRW_OPCODE_ASR] = {
> .name = "asr", .nsrc = 2, .ndst = 1, .gens = GEN_ALL,
> @@ -424,12 +457,14 @@ static const struct opcode_desc opcode_descs[128] = {
> [BRW_OPCODE_JMPI] = {
> .name = "jmpi", .nsrc = 0, .ndst = 0, .gens = GEN_ALL,
> },
> - /* BRW_OPCODE_BRD */
> + [33] = {
> + .name = "brd", .nsrc = 0, .ndst = 0, .gens = GEN_GE(GEN7),
> + },
> [BRW_OPCODE_IF] = {
> .name = "if", .nsrc = 0, .ndst = 0, .gens = GEN_ALL,
> },
> - [BRW_OPCODE_IFF] = { /* also BRW_OPCODE_BRC */
> - .name = "iff", .nsrc = 0, .ndst = 0, .gens = GEN_LE(GEN5),
> + [35] = {
> + .table = opcode_35_descs, .size = ARRAY_SIZE(opcode_35_descs),
> },
> [BRW_OPCODE_ELSE] = {
> .name = "else", .nsrc = 0, .ndst = 0, .gens = GEN_ALL,
> @@ -437,8 +472,8 @@ static const struct opcode_desc opcode_descs[128] = {
> [BRW_OPCODE_ENDIF] = {
> .name = "endif", .nsrc = 0, .ndst = 0, .gens = GEN_ALL,
> },
> - [BRW_OPCODE_DO] = { /* also BRW_OPCODE_CASE */
> - .name = "do", .nsrc = 0, .ndst = 0, .gens = GEN_LE(GEN5),
> + [38] = {
> + .table = opcode_38_descs, .size = ARRAY_SIZE(opcode_38_descs),
> },
> [BRW_OPCODE_WHILE] = {
> .name = "while", .nsrc = 0, .ndst = 0, .gens = GEN_ALL,
> @@ -452,11 +487,21 @@ static const struct opcode_desc opcode_descs[128] = {
> [BRW_OPCODE_HALT] = {
> .name = "halt", .nsrc = 0, .ndst = 0, .gens = GEN_ALL,
> },
> - /* BRW_OPCODE_CALLA */
> - /* BRW_OPCODE_MSAVE / BRW_OPCODE_CALL */
> - /* BRW_OPCODE_MREST / BRW_OPCODE_RET */
> - /* BRW_OPCODE_PUSH / BRW_OPCODE_FORK / BRW_OPCODE_GOTO */
> - /* BRW_OPCODE_POP */
> + [43] = {
> + .name = "calla", .nsrc = 0, .ndst = 0, .gens = GEN_GE(GEN75),
> + },
> + [44] = {
> + .table = opcode_44_descs, .size = ARRAY_SIZE(opcode_44_descs),
> + },
> + [45] = {
> + .table = opcode_45_descs, .size = ARRAY_SIZE(opcode_45_descs),
> + },
> + [46] = {
> + .table = opcode_46_descs, .size = ARRAY_SIZE(opcode_46_descs),
> + },
> + [47] = {
> + .name = "pop", .nsrc = 2, .ndst = 0, .gens = GEN_LE(GEN5),
> + },
> [BRW_OPCODE_WAIT] = {
> .name = "wait", .nsrc = 1, .ndst = 0, .gens = GEN_ALL,
> },
> @@ -557,7 +602,10 @@ static const struct opcode_desc opcode_descs[128] = {
> [BRW_OPCODE_LRP] = {
> .name = "lrp", .nsrc = 3, .ndst = 1, .gens = GEN_GE(GEN6),
> },
> - /* Reserved 93-124 */
> + [93] = {
> + .name = "madm", .nsrc = 3, .ndst = 1, .gens = GEN_GE(GEN8),
> + },
> + /* Reserved 94-124 */
> [BRW_OPCODE_NENOP] = {
> .name = "nenop", .nsrc = 0, .ndst = 0, .gens = GEN45,
> },
> @@ -588,8 +636,17 @@ const struct opcode_desc *
> brw_opcode_desc(const struct brw_device_info *devinfo, enum opcode opcode)
> {
> enum gen gen = gen_from_devinfo(devinfo);
> - if ((opcode_descs[opcode].gens & gen) != 0)
> + if ((opcode_descs[opcode].gens & gen) != 0) {
> return &opcode_descs[opcode];
> - else
> + } 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 &table[i];
> + }
> + }
> + unreachable("not found in table");
This will give you undefined behaviour in release builds when the entry
is not found in the table (e.g. while using it to implement
is_unsupported_inst() in the validator and a multiple-instruction opcode
happens to be unsupported).
> + } else {
> return NULL;
> + }
> }
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h b/src/mesa/drivers/dri/i965/brw_eu.h
> index 30bd903..8a8b7b0 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> @@ -546,8 +546,16 @@ next_offset(const struct brw_device_info *devinfo, void *store, int offset)
> }
>
> struct opcode_desc {
> - char *name;
> - int nsrc;
> + union {
> + struct {
> + char *name;
> + int nsrc;
> + };
> + struct {
> + const struct opcode_desc *table;
> + unsigned size;
> + };
> + };
It's easy to create a breach in the type system by accident when you do
this kind of loosely discriminated unions -- Where is the tag? I
suspect you intended to use the gens field being zero or not as tag?
But *only* for top-level entries I guess? Shouldn't it be documented
clearly somewhere close to the union definition what member applies
under which circumstances?
In fact the code above in brw_opcode_desc() discriminates the union
based on whether 'opcode_descs[opcode].gens & gen' equals zero or not --
But that cannot possibly be right because 'gen' is a runtime parameter
while the table layout is baked in at compile time -- So in cases where
'gen' is not part of the gens bit-set of a given single-opcode entry
you'll end up interpreting the name string as an opcode table and end up
reading garbage and/or crashing -- This is precisely the kind of "joy" I
was referring to while talking about using inconsistent representations
for different opcodes.
Anyway if you can get your implementation to be type-correct feel free
to slap my:
Acked-by: Francisco Jerez <currojerez at riseup.net>
on it. I don't plan on reviewing the series carefully because I
consider the previous proposal to be more elegant and easier to reason
about and maintain, but this code is not important enough for me to
continue the argument so I'm not going to oppose to it as long as
someone else is willing to look through the changes in detail
(especially the manual table merging).
> int ndst;
> int gens;
> };
> --
> 2.7.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160501/937f2daa/attachment-0001.sig>
More information about the mesa-dev
mailing list