[Mesa-dev] [PATCH] i965: expose BRW_OPCODE_[F32TO16/F16TO32] opcode_descs on gen8+

[Alejandro Piñeiro]

On 29/03/17 16:15, Matt Turner wrote:
> On Wed, Mar 29, 2017 at 4:47 AM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
>> Technically those hw operations are only available on gen7, as gen8+
>> support the conversion on the MOV. But, when using the builder to
>> implement nir operations (example: nir_op_fquantize2f16), it is not
>> needed to do the gen check. This check is done later, on the final
>> emission at brw_F32TO16 (brw_eu_emit), choosing between the MOV or the
>> specific operation accordingly.
>>
>> So in the middle, during optimization phases those hw operations can
>> be around for gen8+ too.
>>
>> Without this patch, several (at least 95) vulkan-cts quantize tests
>> crashes when using INTEL_DEBUG=optimizer. For example:
>> dEQP-VK.spirv_assembly.instruction.graphics.opquantize.too_small_vert
>> ---
>>  src/intel/compiler/brw_eu.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_eu.c b/src/intel/compiler/brw_eu.c
>> index 77400c1..bff37d7 100644
>> --- a/src/intel/compiler/brw_eu.c
>> +++ b/src/intel/compiler/brw_eu.c
>> @@ -499,10 +499,10 @@ static const struct opcode_desc opcode_descs[128] = {
>>        .name = "csel",    .nsrc = 3, .ndst = 1, .gens = GEN_GE(GEN8),
>>     },
>>     [BRW_OPCODE_F32TO16] = {
>> -      .name = "f32to16", .nsrc = 1, .ndst = 1, .gens = GEN7 | GEN75,
>> +      .name = "f32to16", .nsrc = 1, .ndst = 1, .gens = GEN7 | GEN75 | GEN8 | GEN9,
>>     },
>>     [BRW_OPCODE_F16TO32] = {
>> -      .name = "f16to32", .nsrc = 1, .ndst = 1, .gens = GEN7 | GEN75,
>> +      .name = "f16to32", .nsrc = 1, .ndst = 1, .gens = GEN7 | GEN75 | GEN8 | GEN9,
>>     },
> This table is for hardware information, used by brw_eu_validate.c.
> Since these opcodes do not exist on Gen8+, we should not add that to
> the table.
>
> I assume that the crashes you are referring to are assertion failures
> in brw_instruction_name() -- assert(brw_opcode_desc(devinfo,
> op)->name)
>
> If that's the case, there's an identical case immediately above. We
> use BRW_OPCODE_DO in the backend IRs, but that opcode is not used on
> Gen6+. I would add two more cases for f32to16 and f16to32 there.

Ok, thanks for the hints. I would work on a v3 of the patch.

> Perhaps we should not use BRW_OPCODE_* for operations used in the
> backend IR that may not actually exist as a real opcode in hardware.
> Not sure.


Yes, at first I found it somewhat counter-intuitive, so I checked just
in case, and it is happening (or happening something really similar)
with several other hw opcodes. The alternative would be create a new
kind of opcode, having hw_opcode and <name>_opcode. But I don't think
that it is worth so such effort, and it is okish to just remember that
there are still a lot happening after calling bld.emit(opcode, ...).

BR