[Mesa-dev] [PATCH 2/5] i965/fs: fix stride and type for hw_reg's in regs_read()

[Jason Ekstrand]

On Wed, Jul 15, 2015 at 4:31 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Connor Abbott <cwabbott0 at gmail.com> writes:
>
>> On Tue, Jul 14, 2015 at 6:02 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Connor Abbott <cwabbott0 at gmail.com> writes:
>>>
>>>> sources with file == HW_REG get all their information from the
>>>> fixed_hw_reg field, so we need to get the stride and type from there
>>>> when computing the size.
>>>>
>>>> Signed-off-by: Connor Abbott <connor.w.abbott at intel.com>
>>>> ---
>>>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 24 ++++++++++++++++++------
>>>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>>> index 38b9095..64f093b 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>>> @@ -696,24 +696,36 @@ fs_inst::regs_read(int arg) const
>>>>        break;
>>>>     }
>>>>
>>>> +   unsigned stride;
>>>> +   enum brw_reg_type type;
>>>> +
>>>>     switch (src[arg].file) {
>>>>     case BAD_FILE:
>>>>     case UNIFORM:
>>>>     case IMM:
>>>>        return 1;
>>>> +
>>>>     case GRF:
>>>> +      stride = src[arg].stride;
>>>> +      type = src[arg].type;
>>>> +      break;
>>>> +
>>>>     case HW_REG:
>>>> -      if (src[arg].stride == 0) {
>>>> -         return 1;
>>>> -      } else {
>>>> -         int size = components * this->exec_size * type_sz(src[arg].type);
>>>> -         return DIV_ROUND_UP(size * src[arg].stride, 32);
>>>> -      }
>>>> +      stride = src[arg].fixed_hw_reg.hstride;
>>>> +      type = src[arg].fixed_hw_reg.type;
>>>> +      break;
>>>> +
>>>>     case MRF:
>>>>        unreachable("MRF registers are not allowed as sources");
>>>>     default:
>>>>        unreachable("Invalid register file");
>>>>     }
>>>> +
>>>> +   if (stride == 0)
>>>> +      return 1;
>>>> +
>>>> +   int size = components * this->exec_size * type_sz(type);
>>>> +   return DIV_ROUND_UP(size * stride, 32);
>>>
>>> I don't think this will work unfortunately, brw_reg::hstride is the log2
>>> of the actual stride, unlike fs_reg::stride.  Did I already mention I'm
>>> appalled by the fact that fs_reg has a number of fields with overlapping
>>> semantics but different representation, one or the other being
>>> applicable depending on the occasion.  I guess it would be more or less
>>> bearable if these data members were declared private and some reasonable
>>> abstraction was provided to access them.
>>
>> I don't think anybody's happy with it, but refactoring that is it's
>> own can of worms.
>>
>
> Sure, it would be a pile of work, but I think it should be quite
> straightforward in principle.  We could just punt fixed_hw_reg and
> replace it with an ARF file and a fixed-GRF file using the same fields
> normal regististers use.  For immediates we'd have to add to add a union
> with float/unsigned/int fields similar to brw_reg::dw1.
>
>>>
>>> How do you like the attached patch?  It doesn't solve the fundamental
>>> problem but it seems to improve the situation slightly.
>>
>> It seems fine to me... I was more paranoid about getting the type from
>> the fixed_hw_reg too, but brw_reg_from_fs_reg() in the generator we
>> have:
>>
>> assert(reg->type == reg->fixed_hw_reg.type);
>>
>> so it seems my paranoia wasn't justified. I'd like someone else who's
>> more experienced to take a look though. I suspect that others might
>> want to bikeshed about the name, but I don't have a better suggestion.

Seems perfectly reasonable to me.  Consider the patch you attached

Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>

There are several more places in the compiler where we completely fall
over on that calculation.  However, this seems like a good start.
--Jason

> Yeah, your paranoia was definitely justified, it's essentially the same
> problem.  It actually led to actual bugs in the past which is why I
> added that assertion and changed retype() to keep the type of the
> fixed_hw_reg in sync with the normal type...
>
>>>
>>>>  }
>>>>
>>>>  bool
>>>> --
>>>> 2.4.3
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>