[Mesa-dev] [PATCH 06/24] i965: Add and use enum brw_reg_file.

[Matt Turner]

On Tue, Nov 3, 2015 at 3:11 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 3 November 2015 at 00:29, Matt Turner <mattst88 at gmail.com> wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_defines.h        | 10 ++++++----
>>  src/mesa/drivers/dri/i965/brw_eu_emit.c        |  2 +-
>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp |  5 +++--
>>  src/mesa/drivers/dri/i965/brw_reg.h            | 25 +++++++++++++------------
>>  4 files changed, 23 insertions(+), 19 deletions(-)
>>
>
> There are a couple of places that can also use the same treatment:
>
> brw_disasm.c:
>
> static int reg(... unsigned _reg_file ...)
>
> static int dest_3src(...)
> ...
> uint32_t reg_file;
> ...
>
> brw_inst.h:
> Worth tweaking the FF/F8 macros (wrt *_reg_file) ?

Yeah, potentially. The main value you get from using enums is better
debugger support and I've never had to debug the disassembler
(fortunately :). It's not uncommon to reach a brw_inst.h function in
gdb, but you just go up a frame to print its argument, so those aren't
a big deal either. I also don't know how much work it'd be to start
supporting different parameter/return types in brw_inst.h

I'm happy to leave both of those projects for later :)

>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> index e980003..ed3e335 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> @@ -33,7 +33,8 @@
>>  #include "brw_fs.h"
>>  #include "brw_cfg.h"
>>
>> -static uint32_t brw_file_from_reg(fs_reg *reg)
>> +static enum brw_reg_file
>> +brw_file_from_reg(fs_reg *reg)
>>  {
>>     switch (reg->file) {
>>     case GRF:
>> @@ -48,7 +49,7 @@ static uint32_t brw_file_from_reg(fs_reg *reg)
>>     case UNIFORM:
>>        unreachable("not reached");
>>     }
>> -   return 0;
>> +   return GRF;
> Although unreachable (and gcc being silly) GRF looks wrong. Use
> BRW_ARCHITECTURE_REGISTER_FILE perhaps ?

*shrug*

Sure.