[Mesa-dev] [PATCH 09/25] i965: Reorder brw_reg_type enum values

[Matt Turner]

On Tue, Aug 8, 2017 at 4:21 PM, Scott D Phillips
<scott.d.phillips at intel.com> wrote:
> Matt Turner <mattst88 at gmail.com> writes:
>> These vaguely corresponded to the hardware encodings, but that is purely
>> historical at this point. Reorder them so we stop making things "almost
>> work" when mixing enums.
>>
>> The ordering has been closen so that no enum value is the same as a
>> compatible hardware encoding.
>> ---
>>  src/intel/compiler/brw_eu.c      |  1 -
>>  src/intel/compiler/brw_eu_emit.c |  6 ------
>>  src/intel/compiler/brw_fs.cpp    |  1 +
>>  src/intel/compiler/brw_reg.h     | 32 ++++++++++++++------------------
>>  src/intel/compiler/brw_vec4.cpp  |  3 ++-
>>  5 files changed, 17 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_eu.c b/src/intel/compiler/brw_eu.c
>> index 0ef52e219c..700a1badd4 100644
>> --- a/src/intel/compiler/brw_eu.c
>> +++ b/src/intel/compiler/brw_eu.c
>> @@ -62,7 +62,6 @@ brw_reg_type_letters(unsigned type)
>>        [BRW_REGISTER_TYPE_UQ] = "UQ",
>>        [BRW_REGISTER_TYPE_Q]  = "Q",
>>     };
>> -   assert(type <= BRW_REGISTER_TYPE_Q);
>>     return names[type];
>>  }
>>
>> diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c
>> index 6673e0741a..b59fc33a54 100644
>> --- a/src/intel/compiler/brw_eu_emit.c
>> +++ b/src/intel/compiler/brw_eu_emit.c
>> @@ -112,7 +112,6 @@ brw_reg_type_to_hw_type(const struct gen_device_info *devinfo,
>>        };
>>        assert(type < ARRAY_SIZE(imm_hw_types));
>>        assert(imm_hw_types[type] != -1);
>> -      assert(devinfo->gen >= 8 || type < BRW_REGISTER_TYPE_DF);
>>        return imm_hw_types[type];
>>     } else {
>>        /* Non-immediate registers */
>> @@ -134,8 +133,6 @@ brw_reg_type_to_hw_type(const struct gen_device_info *devinfo,
>>        };
>>        assert(type < ARRAY_SIZE(hw_types));
>>        assert(hw_types[type] != -1);
>> -      assert(devinfo->gen >= 7 || type < BRW_REGISTER_TYPE_DF);
>> -      assert(devinfo->gen >= 8 || type < BRW_REGISTER_TYPE_Q);
>
> It might be good to keep these asserts around, but phrased as something
> like assert(gen_has_reg_type(gen, file, type)) or something.

I thought about this some. I think the removal is more a feature than a bug.

brw_reg_type_letters is called by dump_instructions().
brw_hw_reg_type_to_size() is called by the disassembler and the
validator. In both of these cases, we really don't want to hit an
assert when we're just trying to generate some textual representation
of the instruction. If it has some bogus type, we want to see it (and
the rest of the instructions) rather than failing an assertion.

brw_reg_type_to_hw_type() is called by functions writing the
instruction word, so it's a different case. My goal has been to move
the unorganized assertions (see brw_MUL for example) into the
structure of brw_eu_validate.c. So I think I'm okay with removing this
ad hoc validation.

>>        return hw_types[type];
>>     }
>>  }
>> @@ -184,9 +181,6 @@ brw_hw_reg_type_to_size(const struct gen_device_info *devinfo,
>>           [GEN8_HW_REG_NON_IMM_TYPE_HF] = 2,
>>        };
>>        assert(type < ARRAY_SIZE(hw_sizes));
>> -      assert(devinfo->gen >= 7 ||
>> -             (type < GEN7_HW_REG_NON_IMM_TYPE_DF || type == BRW_HW_REG_TYPE_F));
>> -      assert(devinfo->gen >= 8 || type <= BRW_HW_REG_TYPE_F);
>>        return hw_sizes[type];
>>     }
>>  }
>> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
>> index fdc30d450c..0ea4c4f1cc 100644
>> --- a/src/intel/compiler/brw_fs.cpp
>> +++ b/src/intel/compiler/brw_fs.cpp
>> @@ -403,6 +403,7 @@ void
>>  fs_reg::init()
>>  {
>>     memset(this, 0, sizeof(*this));
>> +   type = BRW_REGISTER_TYPE_UD;
>>     stride = 1;
>>  }
>>
>> diff --git a/src/intel/compiler/brw_reg.h b/src/intel/compiler/brw_reg.h
>> index 17a51fbd65..48e6fd7b7d 100644
>> --- a/src/intel/compiler/brw_reg.h
>> +++ b/src/intel/compiler/brw_reg.h
>> @@ -203,29 +203,25 @@ brw_mask_for_swizzle(unsigned swz)
>>  }
>>
>>  enum PACKED brw_reg_type {
>
> And maybe add a comment here like this part of the commit message:
>
>  * The ordering has been chosen so that no enum value is the same as a
>  * compatible hardware encoding.
>
> So it's clear that matching some hardware encoding is an anti-goal.

Sure, that's a good idea. Thanks!