[Mesa-dev] [PATCH 8/8] i965: Add disassembler support for remaining opcodes.
Matt Turner
mattst88 at gmail.com
Sun May 1 22:59:27 UTC 2016
On Sun, May 1, 2016 at 1:51 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> 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).
Thanks, you're right. I've changed the unreachable(...) to return NULL locally.
>> + } 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?
Yes, I can do that. I've added the following comment to the struct
just above the 'union {'
+ /* The union is an implementation detail used by brw_opcode_desc() to handle
+ * opcodes that have been reused for different instructions across hardware
+ * generations.
+ *
+ * The gens field acts as a tag. If it is non-zero, name points to a string
+ * containing the instruction mnemonic. If it is zero, the table field is
+ * valid and either points to a secondary opcode_desc table with 'size'
+ * elements or is NULL and no such instruction exists for the opcode.
+ */
>
> 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
Good point. I've corrected that locally by checking .gens != 0 first.
> '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).
Regardless, thank you for your feedback. I appreciate it.
More information about the mesa-dev
mailing list