[Mesa-dev] [PATCH 08/14] i965: Prepare for using the ATTR register file in the fs backend

[Kristian Høgsberg]

On Tue, Oct 28, 2014 at 4:33 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Tue, Oct 28, 2014 at 3:17 PM, Kristian Høgsberg <krh at bitplanet.net> wrote:
>> The scalar vertex shader will use the ATTR register file for vertex
>> attributes.  This patch adds support for the ATTR file to fs_visitor.
>>
>> Signed-off-by: Kristian Høgsberg <krh at bitplanet.net>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp           | 12 ++++++++++--
>>  src/mesa/drivers/dri/i965/brw_fs.h             |  3 +++
>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp |  2 ++
>>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp   | 11 +++++++++--
>>  4 files changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 815c8c2..e8819ef 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -76,7 +76,7 @@ fs_inst::init(enum opcode opcode, uint8_t exec_size, const fs_reg &dst,
>>           this->exec_size = dst.width;
>>        } else {
>>           for (int i = 0; i < sources; ++i) {
>> -            if (src[i].file != GRF)
>> +            if (src[i].file != GRF && src[i].file != ATTR)
>>                 continue;
>>
>>              if (this->exec_size <= 1)
>> @@ -97,6 +97,7 @@ fs_inst::init(enum opcode opcode, uint8_t exec_size, const fs_reg &dst,
>>           break;
>>        case GRF:
>>        case HW_REG:
>> +      case ATTR:
>>           assert(this->src[i].width > 0);
>>           if (this->src[i].width == 1) {
>>              this->src[i].effective_width = this->exec_size;
>> @@ -121,6 +122,7 @@ fs_inst::init(enum opcode opcode, uint8_t exec_size, const fs_reg &dst,
>>     case GRF:
>>     case HW_REG:
>>     case MRF:
>> +   case ATTR:
>>        this->regs_written = (dst.width * dst.stride * type_sz(dst.type) + 31) / 32;
>>        break;
>>     case BAD_FILE:
>> @@ -636,7 +638,7 @@ fs_reg::is_contiguous() const
>>  bool
>>  fs_reg::is_valid_3src() const
>>  {
>> -   return file == GRF || file == UNIFORM;
>> +   return file == GRF || file == UNIFORM || file == ATTR;
>>  }
>>
>>  int
>> @@ -3148,6 +3150,9 @@ fs_visitor::dump_instruction(backend_instruction *be_inst, FILE *file)
>>     case UNIFORM:
>>        fprintf(file, "***u%d***", inst->dst.reg + inst->dst.reg_offset);
>>        break;
>> +   case ATTR:
>> +      fprintf(file, "a%d", inst->dst.reg + inst->dst.reg_offset);
>
> a0 is an address register, not this. Print these like the vec4 code
> does -- attr%d.

Ugh, right.

>> +      break;
>>     case HW_REG:
>>        if (inst->dst.fixed_hw_reg.file == BRW_ARCHITECTURE_REGISTER_FILE) {
>>           switch (inst->dst.fixed_hw_reg.nr) {
>> @@ -3199,6 +3204,9 @@ fs_visitor::dump_instruction(backend_instruction *be_inst, FILE *file)
>>        case MRF:
>>           fprintf(file, "***m%d***", inst->src[i].reg);
>>           break;
>> +      case ATTR:
>> +         fprintf(file, "a%d", inst->src[i].reg + inst->src[i].reg_offset);
>
> and here.
>
>> +         break;
>>        case UNIFORM:
>>           fprintf(file, "u%d", inst->src[i].reg + inst->src[i].reg_offset);
>>           if (inst->src[i].reladdr) {
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>> index 67a5cdd..8d60544 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -132,6 +132,7 @@ byte_offset(fs_reg reg, unsigned delta)
>>     case BAD_FILE:
>>        break;
>>     case GRF:
>> +   case ATTR:
>>        reg.reg_offset += delta / 32;
>>        break;
>>     case MRF:
>> @@ -157,6 +158,7 @@ horiz_offset(fs_reg reg, unsigned delta)
>>        break;
>>     case GRF:
>>     case MRF:
>> +   case ATTR:
>>        return byte_offset(reg, delta * reg.stride * type_sz(reg.type));
>>     default:
>>        assert(delta == 0);
>> @@ -173,6 +175,7 @@ offset(fs_reg reg, unsigned delta)
>>        break;
>>     case GRF:
>>     case MRF:
>> +   case ATTR:
>>        return byte_offset(reg, delta * reg.width * reg.stride * type_sz(reg.type));
>>     case UNIFORM:
>>        reg.reg_offset += delta;
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> index a463386..74fe79c 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> @@ -1272,6 +1272,8 @@ brw_reg_from_fs_reg(fs_reg *reg)
>>        break;
>>     case UNIFORM:
>>        unreachable("not reached");
>> +   case ATTR:
>> +      unreachable("not reached");
>
> How about
>
>    case UNIFORM:
>    case ATTR:
>    default:
>       unreachable("not reached");
>
> instead?
>
>>     default:
>>        unreachable("not reached");
>>     }

Yeah... I think the idea with having different cases for the different
unexpected values is that you can see which one it is from the
assertion failure (from the line number).  But in that case you'll
want to run it under gdb anyway, so I'll just leave only the default
case (like brw_file_from_reg above).

Kristian