[Mesa-dev] [PATCH] i965: Respect stride and subreg_offset for ATTR registers

[Jason Ekstrand]

On Wed, Sep 23, 2015 at 5:29 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Wed, Sep 23, 2015 at 5:25 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> On Wed, Sep 23, 2015 at 5:11 PM, Kristian Høgsberg Kristensen
>> <krh at bitplanet.net> wrote:
>>> When we assign hw regs to attributes, we don't incorporate the stride
>>> and subreg_offset from the fs_reg. It's rarely used, but the integer
>>> multiplication lowering uses unusual stride and subreg_offset
>>> combination breaks when one source is an attribute.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91970
>>> Cc: "11.0" <mesa-stable at lists.freedesktop.org>
>>> Signed-off-by: Kristian Høgsberg Kristensen <krh at bitplanet.net>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> index 225a312..618bbd2 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> @@ -1564,7 +1564,10 @@ fs_visitor::assign_vs_urb_setup()
>>>
>>>              inst->src[i].file = HW_REG;
>>>              inst->src[i].fixed_hw_reg =
>>> -               retype(brw_vec8_grf(grf, 0), inst->src[i].type);
>>> +               stride(byte_offset(retype(brw_vec8_grf(grf, 0), inst->src[i].type),
>>> +                                  inst->src[i].subreg_offset),
>>> +                      inst->exec_size * inst->src[i].stride,
>>> +                      inst->exec_size, inst->src[i].stride);
>>
>> I made this comment to Kristian today in the office, but I figure I'll
>> also put it on the list:
>>
>> I'm not 100% convinced that I like having subreg offsets and strides
>> respected on the ATTR.  We do respect subreg offset on the UNIFORM
>> file so I guess there's some precedent for it.  If we didn't, another
>> option would be to simply emit a MOV to a grf during lowering if we
>> ever detect ATTR.  Given that this is only on CHV and probably not hit
>> that often, I think that would be my minor preference.  That said,
>> this patch does work.
>
> IIRC you discussed uniforms in this context with Curro on-list and he
> wasn't in favor of it, but I may be misremembering.
>
> I don't understand what benefit would be gained by ignoring subreg
> offset (and I don't see that it's stated anywhere).

I don't know that there is.  It's mostly me wondering if it actually
makes semantic sense.  It probably does if that sense is "the offset
we get when we lower to a hardware reg".  I don't really care enough
to oppose respecting it that hard.
--Jason