[Mesa-dev] [PATCH v2 16/19] i965/fs: Use the builder dispatch_width for computing register offsets

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Fri, Jun 26, 2015 at 11:51 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>
>>> On Fri, Jun 26, 2015 at 8:52 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>>>
>>>>> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>>>>> ---
>>>>>  src/mesa/drivers/dri/i965/brw_fs.h | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>>>>> index d4cc43d..d94a842 100644
>>>>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>>>>> @@ -72,7 +72,7 @@ offset(fs_reg reg, const brw::fs_builder& bld, unsigned delta)
>>>>>     case MRF:
>>>>>     case ATTR:
>>>>>        return byte_offset(reg,
>>>>> -                         delta * MAX2(reg.width * reg.stride, 1) *
>>>>> +                         delta * bld.dispatch_width() * reg.stride *
>>>>
>>>> Er...  This doesn't look right for stride == 0.  If you keep the
>>>> MAX2(.., 1) expression this patch is:
>>>
>>> I don't think offset() even makes sense for something with stride ==
>>> 0.  I added "assert(stride != 0)" right above the byte_offset() call
>>> and it passed Jenkins.  Would that be an acceptable alternative?
>>
>> stride == 0 implies that each logical component of your vector takes 1
>> scalar component (because all the N channels of your SIMDN value are one
>> and the same scalar in your register file), that means that logically
>> independent components of a vector or array are stored 1 scalar apart,
>> and the previous code was doing the right thing.
>
> I still think offset() is bogus for stride == 0.  However, I don't
> really feel like arguing the point, so I added the MAX2().

No need to argue about it, let me explain it step by step:

In the FS back-end (SoA) registers are in general a sequence of SIMDN
values, each SIMDN value being itself a sequence of N per-channel scalar
values.

Agreed?

offset(reg, i) returns another register reg' based on the i-th SIMDN
value from the start of reg.  [IOW if reg logically represented a vector
(say in a high-level language like GLSL) reg' would be at the i-th
logical component of the the vector]

Agreed?

offset(reg, i) is well-defined as long as the size of a single SIMDN
value is well-defined in the register file, because logically
independent elements of a SoA sequence of SIMDN values are simply stored
contiguously.

Agreed?

The size of a SIMDN value with stride=0 (i.e. a uniform) in the register
file is the same as the size of a single scalar value.  [And, although
it's irrelevant here, the size of a SIMDN value with stride!=0 is
stride*type_sz(type)]

Agreed?

If you agreed with the last two points, you'll also agree that
offset(reg, i) is well-defined for reg.stride=0.  If you're still not
convinced stop for a moment and consider the natural layout of an array
of uniforms in the GRF, and what would be the natural way to pick the
i-th component from such an array.

> --Jason
>
>>> --Jason
>>>
>>>> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>>>>
>>>>>                           type_sz(reg.type));
>>>>>     case UNIFORM:
>>>>>        reg.reg_offset += delta;
>>>>> --
>>>>> 2.4.3
>>>>>
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150701/d4624dc1/attachment.sig>