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. > 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 >>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
