On Wed, Jul 15, 2015 at 4:31 AM, Francisco Jerez <curroje...@riseup.net> wrote:
> Connor Abbott <cwabbo...@gmail.com> writes:
>
>> On Tue, Jul 14, 2015 at 6:02 AM, Francisco Jerez <curroje...@riseup.net>
>> wrote:
>>> Connor Abbott <cwabbo...@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.abb...@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.ekstr...@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@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
