Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
Matt Turner writes: > On Thu, Feb 11, 2016 at 11:21 PM, Francisco Jerez > wrote: >> Matt Turner writes: >> >>> On Thu, Feb 11, 2016 at 7:31 PM, Francisco Jerez >>> wrote: Matt Turner writes: > On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez > wrote: >> Would be really nice if we could also get rid of reg_offset as we're at >> it. reg and subreg_offset basically represent the same thing but with >> different units, couldn't we just have a single offset field in bytes? >> Should it be part of brw_reg or backend_reg? I think I would lean >> towards backend_reg. In that case does it make sense to move this into >> brw_reg now only to move it back to backend_reg later on? > > That would be nice. > > I'm just not sure how to do it. brw_reg has to have the subnr field, > and it's nice if that's the field the higher levels use too. > I guess at this point brw_reg is just an implementation detail of backend_reg, if some of it doesn't make sense at the IR level (e.g. because the IR wants more than 5 bits of sub-(V)GRF offset) there's no need to keep the IR tied up to the lower-level brw_reg representation. >>> >>> Do you have an example of where we might want a subreg_offset >= 32? >> >> reg_offset is basically a subreg_offset in 32B units, any use of >> reg_offset is a good example I guess. ;) > > No, that's circular reasoning :) > How so? Because they have exactly the same semantics but subreg_offset has lower granularity, every use-case for reg_offset is a legitimate use-case of subreg_offset. >>> >>> I think using brw_reg is nice... it pretty simply contains the bits >>> that are common to the IR and the hardware. I'm not finding this limiting. >> >> I don't think it's limiting, but it's silly and error-prone to have two >> different fields with the exact same semantics but different units. It >> means anytime you need to find out what a register reads or writes you >> need to add two terms, and anytime you need to specify a register region >> you need to split up the offset in two terms (or use some of the helpers >> we have for that purpose, e.g. byte_offset(), *or* assume that your >> offset is a multiple of 32b as some places do which will blow up when we >> start doing sub-dword types more extensively). > > FWIW, I haven't found the behavior of byte_offset -- that it > increments reg_offset if we go past the end of the register -- to be > useful. Do we actually rely on that somewhere? That's more in line > with my previous question. > Without byte_offset() you have no sane way to do things like "give me the n-th scalar element from this register region" (several places indeed do it by poking the offset directly instead of going through the helper and they're all rather dubious if not outright broken, see e.g. fs_reg::set_smear() (which blows up for widths greater than 8 or types larger than a dword) or lower_integer_multiplication() which overwrites the subreg_offset of the destination without considering what the previous offset of the region was). > I understand the argument that "they have the same semantics", but I > don't know that we gain anything by combining them, Again, by combining them we spare ourselves from considering two terms rather than one for the *same* thing anytime we need to find out or specify what region of the (virtual) register file a backend_reg refers to. If you need some examples of why this is annoying and repetitive search for the many 'x.reg_offset * 32 + x.subreg_offset' in the backend: brw_fs_copy_propagation.cpp:355 brw_fs_copy_propagation.cpp:469 brw_fs_copy_propagation.cpp:471-472 brw_fs_copy_propagation.cpp:526 brw_fs.cpp:1650 [The list is by no means comprehensive, I just got bored of reading through the grep results] There are also some examples of places where we take into account reg_offset but don't take into account subreg_offset which illustrate what I meant by "error-prone". Was the omission intentional? How is the assumption justified that a region is 32B-aligned? Is it a bug? brw_fs_copy_propagation.cpp:354 brw_fs_copy_propagation.cpp:525 brw_fs.cpp:363 brw_fs.cpp:377 brw_fs.cpp:1496 ... > and also one of the two is a hardware concept, and the other is an IR > concept. Having them separate seems fine to me... The split between reg_ and subreg_offset might be a hardware concept, but it's of little use at the IR level and only a source of bugs in a world of variable SIMD-width and execution types. And our split between the two doesn't even match the hardware's. For the hardware a register is always 32B, while the reg_offset unit is in fact dependent on the register file in a way I won't claim to remember exactly (what leads to a number of dubious assert() calls that wouldn't be there if offsets were represented consistently across register files, see e.g. byte_offset()). Thankfully it's at least no longer dep
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
On Thu, Feb 11, 2016 at 11:21 PM, Francisco Jerez wrote: > Matt Turner writes: > >> On Thu, Feb 11, 2016 at 7:31 PM, Francisco Jerez >> wrote: >>> Matt Turner writes: >>> On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez wrote: > Would be really nice if we could also get rid of reg_offset as we're at > it. reg and subreg_offset basically represent the same thing but with > different units, couldn't we just have a single offset field in bytes? > Should it be part of brw_reg or backend_reg? I think I would lean > towards backend_reg. In that case does it make sense to move this into > brw_reg now only to move it back to backend_reg later on? That would be nice. I'm just not sure how to do it. brw_reg has to have the subnr field, and it's nice if that's the field the higher levels use too. >>> I guess at this point brw_reg is just an implementation detail of >>> backend_reg, if some of it doesn't make sense at the IR level >>> (e.g. because the IR wants more than 5 bits of sub-(V)GRF offset) >>> there's no need to keep the IR tied up to the lower-level brw_reg >>> representation. >> >> Do you have an example of where we might want a subreg_offset >= 32? > > reg_offset is basically a subreg_offset in 32B units, any use of > reg_offset is a good example I guess. ;) No, that's circular reasoning :) >> >> I think using brw_reg is nice... it pretty simply contains the bits >> that are common to the IR and the hardware. I'm not finding this limiting. > > I don't think it's limiting, but it's silly and error-prone to have two > different fields with the exact same semantics but different units. It > means anytime you need to find out what a register reads or writes you > need to add two terms, and anytime you need to specify a register region > you need to split up the offset in two terms (or use some of the helpers > we have for that purpose, e.g. byte_offset(), *or* assume that your > offset is a multiple of 32b as some places do which will blow up when we > start doing sub-dword types more extensively). FWIW, I haven't found the behavior of byte_offset -- that it increments reg_offset if we go past the end of the register -- to be useful. Do we actually rely on that somewhere? That's more in line with my previous question. I understand the argument that "they have the same semantics", but I don't know that we gain anything by combining them, and also one of the two is a hardware concept, and the other is an IR concept. Having them separate seems fine to me... ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
On Thu, 2016-02-11 at 19:12 -0800, Kenneth Graunke wrote: > On Thursday, February 11, 2016 5:49:55 PM PST Matt Turner wrote: > > On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez > wrote: > > > Would be really nice if we could also get rid of reg_offset as we're at > > > it. reg and subreg_offset basically represent the same thing but with > > > different units, couldn't we just have a single offset field in bytes? > > > Should it be part of brw_reg or backend_reg? I think I would lean > > > towards backend_reg. In that case does it make sense to move this into > > > brw_reg now only to move it back to backend_reg later on? > > > > That would be nice. > > > > I'm just not sure how to do it. brw_reg has to have the subnr field, > > and it's nice if that's the field the higher levels use too. > > > > I wonder -- is it possible that we could just get rid of reg_offset > > too? For gathering data we have load_payload, so it's not useful > > there. I think it's mainly useful for accessing elements of texturing > > results. Is doubt there is a way we could avoid that though? > > I disagree - I don't think this would be nice at all. > > When we designed the IR, we needed something to handle cases like > texturing results, where we actually need to store a whole vec4, > and can't break it into separate scalar components. (Note that > messages used MRFs; we didn't even know about send-from-GRF.) > > To handle this, Eric created an abstraction of "virtual registers with > size > 1", where we basically have an array of registers, each of which > holds a single scalar value. It can be thought of as a vecN, and > reg_offset is the array index - v[i] - selecting each of the logically > contiguous components. reg_offset only makes sense on these large > virtual registers - it has no meaning for real hardware registers. > > In contrast, subreg_offset (which came later) offered a way to access > particular channels of a single hardware register, such as g0.2. We > added this later to make the IR more expressive. > > While these are both offsets, they serve very different purposes. > > Replacing subreg_offset with subnr makes a lot of sense to me, as > both are basically a way to provide a byte offset for the start of > a register region, allowing unaligned register access. But reg_offset > is a different beast. > > If you want to be rid of it, then perhaps we should consider removing > the "VGRF of size > 1" abstraction. One could imagine a system where > we allocate separate VRFs for each scalar value, but record that > "VRFs 4, 12, and 63 need to be contiguous", passing that information > to the register allocator. There are certainly other fine approaches. > > I would also humbly request that you wait until FP64 lands before making > any major changes. It's very easy to conflate type size, number of SIMD > channels, and register offsets, and a lot of the FP64 work is fixing > places that are confused about that. I'd really like to avoid making > our Igalia friends' lives harder by making them rebase 100 patches on > IR redesigns. Thanks Ken for thinking about us :) I would definitely appreciate this! The patch count for fp64 is already above 150 and rebasing it on top of changes like these sounds quite scary to me. Iago ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
Matt Turner writes: > On Thu, Feb 11, 2016 at 7:31 PM, Francisco Jerez > wrote: >> Matt Turner writes: >> >>> On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez >>> wrote: Would be really nice if we could also get rid of reg_offset as we're at it. reg and subreg_offset basically represent the same thing but with different units, couldn't we just have a single offset field in bytes? Should it be part of brw_reg or backend_reg? I think I would lean towards backend_reg. In that case does it make sense to move this into brw_reg now only to move it back to backend_reg later on? >>> >>> That would be nice. >>> >>> I'm just not sure how to do it. brw_reg has to have the subnr field, >>> and it's nice if that's the field the higher levels use too. >>> >> I guess at this point brw_reg is just an implementation detail of >> backend_reg, if some of it doesn't make sense at the IR level >> (e.g. because the IR wants more than 5 bits of sub-(V)GRF offset) >> there's no need to keep the IR tied up to the lower-level brw_reg >> representation. > > Do you have an example of where we might want a subreg_offset >= 32? reg_offset is basically a subreg_offset in 32B units, any use of reg_offset is a good example I guess. ;) > > I think using brw_reg is nice... it pretty simply contains the bits > that are common to the IR and the hardware. I'm not finding this limiting. I don't think it's limiting, but it's silly and error-prone to have two different fields with the exact same semantics but different units. It means anytime you need to find out what a register reads or writes you need to add two terms, and anytime you need to specify a register region you need to split up the offset in two terms (or use some of the helpers we have for that purpose, e.g. byte_offset(), *or* assume that your offset is a multiple of 32b as some places do which will blow up when we start doing sub-dword types more extensively). signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
On Thu, Feb 11, 2016 at 7:31 PM, Francisco Jerez wrote: > Matt Turner writes: > >> On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez >> wrote: >>> Would be really nice if we could also get rid of reg_offset as we're at >>> it. reg and subreg_offset basically represent the same thing but with >>> different units, couldn't we just have a single offset field in bytes? >>> Should it be part of brw_reg or backend_reg? I think I would lean >>> towards backend_reg. In that case does it make sense to move this into >>> brw_reg now only to move it back to backend_reg later on? >> >> That would be nice. >> >> I'm just not sure how to do it. brw_reg has to have the subnr field, >> and it's nice if that's the field the higher levels use too. >> > I guess at this point brw_reg is just an implementation detail of > backend_reg, if some of it doesn't make sense at the IR level > (e.g. because the IR wants more than 5 bits of sub-(V)GRF offset) > there's no need to keep the IR tied up to the lower-level brw_reg > representation. Do you have an example of where we might want a subreg_offset >= 32? I think using brw_reg is nice... it pretty simply contains the bits that are common to the IR and the hardware. I'm not finding this limiting. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
Matt Turner writes: > On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez > wrote: >> Would be really nice if we could also get rid of reg_offset as we're at >> it. reg and subreg_offset basically represent the same thing but with >> different units, couldn't we just have a single offset field in bytes? >> Should it be part of brw_reg or backend_reg? I think I would lean >> towards backend_reg. In that case does it make sense to move this into >> brw_reg now only to move it back to backend_reg later on? > > That would be nice. > > I'm just not sure how to do it. brw_reg has to have the subnr field, > and it's nice if that's the field the higher levels use too. > I guess at this point brw_reg is just an implementation detail of backend_reg, if some of it doesn't make sense at the IR level (e.g. because the IR wants more than 5 bits of sub-(V)GRF offset) there's no need to keep the IR tied up to the lower-level brw_reg representation. > I wonder -- is it possible that we could just get rid of reg_offset > too? For gathering data we have load_payload, so it's not useful > there. I think it's mainly useful for accessing elements of texturing > results. Is doubt there is a way we could avoid that though? signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
Kenneth Graunke writes: > On Thursday, February 11, 2016 5:49:55 PM PST Matt Turner wrote: >> On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez > wrote: >> > Would be really nice if we could also get rid of reg_offset as we're at >> > it. reg and subreg_offset basically represent the same thing but with >> > different units, couldn't we just have a single offset field in bytes? >> > Should it be part of brw_reg or backend_reg? I think I would lean >> > towards backend_reg. In that case does it make sense to move this into >> > brw_reg now only to move it back to backend_reg later on? >> >> That would be nice. >> >> I'm just not sure how to do it. brw_reg has to have the subnr field, >> and it's nice if that's the field the higher levels use too. >> >> I wonder -- is it possible that we could just get rid of reg_offset >> too? For gathering data we have load_payload, so it's not useful >> there. I think it's mainly useful for accessing elements of texturing >> results. Is doubt there is a way we could avoid that though? > > I disagree - I don't think this would be nice at all. > > When we designed the IR, we needed something to handle cases like > texturing results, where we actually need to store a whole vec4, > and can't break it into separate scalar components. (Note that > messages used MRFs; we didn't even know about send-from-GRF.) > > To handle this, Eric created an abstraction of "virtual registers with > size > 1", where we basically have an array of registers, each of which > holds a single scalar value. It can be thought of as a vecN, and > reg_offset is the array index - v[i] - selecting each of the logically > contiguous components. reg_offset only makes sense on these large > virtual registers - it has no meaning for real hardware registers. > > In contrast, subreg_offset (which came later) offered a way to access > particular channels of a single hardware register, such as g0.2. We > added this later to make the IR more expressive. > > While these are both offsets, they serve very different purposes. > That hasn't been the case for a while, reg_offset is expressed in multiples of a GRF, IOW it's just subreg_offset in different units, there's no point in keeping both of them around, and it makes things unnecessarily difficult and error-prone to have to take both of them into account anytime you need to find out e.g. what region of a VGRF a backend_reg refers to. One of them needs to die IMO. > Replacing subreg_offset with subnr makes a lot of sense to me, as > both are basically a way to provide a byte offset for the start of > a register region, allowing unaligned register access. But reg_offset > is a different beast. > > If you want to be rid of it, then perhaps we should consider removing > the "VGRF of size > 1" abstraction. One could imagine a system where > we allocate separate VRFs for each scalar value, but record that > "VRFs 4, 12, and 63 need to be contiguous", passing that information > to the register allocator. There are certainly other fine approaches. > I don't think I see what the benefit would be of replacing multi-component VGRFs with such a thing. Definitely a lot more work than unifying reg_ and subreg_offset. > I would also humbly request that you wait until FP64 lands before making > any major changes. It's very easy to conflate type size, number of SIMD > channels, and register offsets, and a lot of the FP64 work is fixing > places that are confused about that. I'd really like to avoid making > our Igalia friends' lives harder by making them rebase 100 patches on > IR redesigns. > > --Ken signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
On Thursday, February 11, 2016 5:49:55 PM PST Matt Turner wrote: > On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez wrote: > > Would be really nice if we could also get rid of reg_offset as we're at > > it. reg and subreg_offset basically represent the same thing but with > > different units, couldn't we just have a single offset field in bytes? > > Should it be part of brw_reg or backend_reg? I think I would lean > > towards backend_reg. In that case does it make sense to move this into > > brw_reg now only to move it back to backend_reg later on? > > That would be nice. > > I'm just not sure how to do it. brw_reg has to have the subnr field, > and it's nice if that's the field the higher levels use too. > > I wonder -- is it possible that we could just get rid of reg_offset > too? For gathering data we have load_payload, so it's not useful > there. I think it's mainly useful for accessing elements of texturing > results. Is doubt there is a way we could avoid that though? I disagree - I don't think this would be nice at all. When we designed the IR, we needed something to handle cases like texturing results, where we actually need to store a whole vec4, and can't break it into separate scalar components. (Note that messages used MRFs; we didn't even know about send-from-GRF.) To handle this, Eric created an abstraction of "virtual registers with size > 1", where we basically have an array of registers, each of which holds a single scalar value. It can be thought of as a vecN, and reg_offset is the array index - v[i] - selecting each of the logically contiguous components. reg_offset only makes sense on these large virtual registers - it has no meaning for real hardware registers. In contrast, subreg_offset (which came later) offered a way to access particular channels of a single hardware register, such as g0.2. We added this later to make the IR more expressive. While these are both offsets, they serve very different purposes. Replacing subreg_offset with subnr makes a lot of sense to me, as both are basically a way to provide a byte offset for the start of a register region, allowing unaligned register access. But reg_offset is a different beast. If you want to be rid of it, then perhaps we should consider removing the "VGRF of size > 1" abstraction. One could imagine a system where we allocate separate VRFs for each scalar value, but record that "VRFs 4, 12, and 63 need to be contiguous", passing that information to the register allocator. There are certainly other fine approaches. I would also humbly request that you wait until FP64 lands before making any major changes. It's very easy to conflate type size, number of SIMD channels, and register offsets, and a lot of the FP64 work is fixing places that are confused about that. I'd really like to avoid making our Igalia friends' lives harder by making them rebase 100 patches on IR redesigns. --Ken signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez wrote: > Would be really nice if we could also get rid of reg_offset as we're at > it. reg and subreg_offset basically represent the same thing but with > different units, couldn't we just have a single offset field in bytes? > Should it be part of brw_reg or backend_reg? I think I would lean > towards backend_reg. In that case does it make sense to move this into > brw_reg now only to move it back to backend_reg later on? That would be nice. I'm just not sure how to do it. brw_reg has to have the subnr field, and it's nice if that's the field the higher levels use too. I wonder -- is it possible that we could just get rid of reg_offset too? For gathering data we have load_payload, so it's not useful there. I think it's mainly useful for accessing elements of texturing results. Is doubt there is a way we could avoid that though? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
Kenneth Graunke writes:
> On Thursday, February 11, 2016 1:49:21 PM PST Matt Turner wrote:
>> ---
>> src/mesa/drivers/dri/i965/brw_fs.cpp | 31 ++
> +---
>> .../drivers/dri/i965/brw_fs_combine_constants.cpp | 13 +
>> .../drivers/dri/i965/brw_fs_copy_propagation.cpp | 14 +-
>> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 2 +-
>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 9 ++-
>> src/mesa/drivers/dri/i965/brw_ir_fs.h | 13 +++--
>> 6 files changed, 35 insertions(+), 47 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/
> i965/brw_fs.cpp
>> index 41a3f81..6ee590e 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -432,7 +432,6 @@ fs_reg::fs_reg(struct ::brw_reg reg) :
>> backend_reg(reg)
>> {
>> this->reg_offset = 0;
>> - this->subreg_offset = 0;
>> this->reladdr = NULL;
>> this->stride = 1;
>> if (this->file == IMM &&
>> @@ -447,7 +446,6 @@ bool
>> fs_reg::equals(const fs_reg &r) const
>> {
>> return (this->backend_reg::equals(r) &&
>> - subreg_offset == r.subreg_offset &&
>> !reladdr && !r.reladdr &&
>> stride == r.stride);
>> }
>> @@ -456,7 +454,8 @@ fs_reg &
>> fs_reg::set_smear(unsigned subreg)
>> {
>> assert(file != ARF && file != FIXED_GRF && file != IMM);
>> - subreg_offset = subreg * type_sz(type);
>> + assert(subreg * type_sz(type) < (1 << 5)); /* subnr is 5 bits */
>> + subnr = subreg * type_sz(type);
>> stride = 0;
>> return *this;
>> }
>> @@ -1513,7 +1512,7 @@ fs_visitor::assign_curb_setup()
>> assert(inst->src[i].stride == 0);
>> inst->src[i] = byte_offset(
>> retype(brw_reg, inst->src[i].type),
>> - inst->src[i].subreg_offset);
>> + inst->src[i].subnr);
>> }
>>}
>> }
>> @@ -1653,7 +1652,7 @@ fs_visitor::convert_attr_sources_to_hw_regs(fs_inst
> *inst)
>> unsigned width = inst->src[i].stride == 0 ? 1 : inst->exec_size;
>> struct brw_reg reg =
>> stride(byte_offset(retype(brw_vec8_grf(grf, 0), inst-
>>src[i].type),
>> - inst->src[i].subreg_offset),
>> + inst->src[i].subnr),
>> inst->exec_size * inst->src[i].stride,
>> width, inst->src[i].stride);
>> reg.abs = inst->src[i].abs;
>> @@ -2597,7 +2596,7 @@ fs_visitor::compute_to_mrf()
>>inst->dst.type != inst->src[0].type ||
>>inst->src[0].abs || inst->src[0].negate ||
>>!inst->src[0].is_contiguous() ||
>> - inst->src[0].subreg_offset)
>> + inst->src[0].subnr)
>> continue;
>>
>>/* Work out which hardware MRF registers are written by this
>> @@ -3367,7 +3366,7 @@ fs_visitor::lower_integer_multiplication()
>> assert(src1_1_w.stride == 1);
>> src1_1_w.stride = 2;
>>}
>> - src1_1_w.subreg_offset += type_sz(BRW_REGISTER_TYPE_UW);
>> + src1_1_w.subnr += type_sz(BRW_REGISTER_TYPE_UW);
>> }
>> ibld.MUL(low, inst->src[0], src1_0_w);
>> ibld.MUL(high, inst->src[0], src1_1_w);
>> @@ -3386,7 +3385,7 @@ fs_visitor::lower_integer_multiplication()
>>assert(src0_1_w.stride == 1);
>>src0_1_w.stride = 2;
>> }
>> - src0_1_w.subreg_offset += type_sz(BRW_REGISTER_TYPE_UW);
>> + src0_1_w.subnr += type_sz(BRW_REGISTER_TYPE_UW);
>>
>> ibld.MUL(low, src0_0_w, inst->src[1]);
>> ibld.MUL(high, src0_1_w, inst->src[1]);
>> @@ -3394,14 +3393,14 @@ fs_visitor::lower_integer_multiplication()
>>
>> fs_reg dst = inst->dst;
>> dst.type = BRW_REGISTER_TYPE_UW;
>> -dst.subreg_offset = 2;
>> +dst.subnr = 2;
>> dst.stride = 2;
>>
>> high.type = BRW_REGISTER_TYPE_UW;
>> high.stride = 2;
>>
>> low.type = BRW_REGISTER_TYPE_UW;
>> -low.subreg_offset = 2;
>> +low.subnr = 2;
>> low.stride = 2;
>>
>> ibld.ADD(dst, low, high);
>> @@ -4642,9 +4641,9 @@ fs_visitor::dump_instruction(backend_instruction
> *be_inst, FILE *file)
>> case VGRF:
>>fprintf(file, "vgrf%d", inst->dst.nr);
>>if (alloc.sizes[inst->dst.nr] != inst->regs_written ||
>> - inst->dst.subreg_offset)
>> + inst->dst.subnr)
>> fprintf(file, "+%d.%d",
>> - inst->dst.reg_offset, inst->dst.subreg_offset);
>> + inst->dst.reg_offset, inst->dst.subnr);
>>break;
>> case FIXED_GRF:
>>fprintf(file, "g%d", inst->dst.nr);
>> @@ -4698,9 +4697,9 @@ fs_visitor::dump_instruc
Re: [Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
On Thursday, February 11, 2016 1:49:21 PM PST Matt Turner wrote:
> ---
> src/mesa/drivers/dri/i965/brw_fs.cpp | 31 ++
+---
> .../drivers/dri/i965/brw_fs_combine_constants.cpp | 13 +
> .../drivers/dri/i965/brw_fs_copy_propagation.cpp | 14 +-
> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 2 +-
> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 9 ++-
> src/mesa/drivers/dri/i965/brw_ir_fs.h | 13 +++--
> 6 files changed, 35 insertions(+), 47 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/
i965/brw_fs.cpp
> index 41a3f81..6ee590e 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -432,7 +432,6 @@ fs_reg::fs_reg(struct ::brw_reg reg) :
> backend_reg(reg)
> {
> this->reg_offset = 0;
> - this->subreg_offset = 0;
> this->reladdr = NULL;
> this->stride = 1;
> if (this->file == IMM &&
> @@ -447,7 +446,6 @@ bool
> fs_reg::equals(const fs_reg &r) const
> {
> return (this->backend_reg::equals(r) &&
> - subreg_offset == r.subreg_offset &&
> !reladdr && !r.reladdr &&
> stride == r.stride);
> }
> @@ -456,7 +454,8 @@ fs_reg &
> fs_reg::set_smear(unsigned subreg)
> {
> assert(file != ARF && file != FIXED_GRF && file != IMM);
> - subreg_offset = subreg * type_sz(type);
> + assert(subreg * type_sz(type) < (1 << 5)); /* subnr is 5 bits */
> + subnr = subreg * type_sz(type);
> stride = 0;
> return *this;
> }
> @@ -1513,7 +1512,7 @@ fs_visitor::assign_curb_setup()
> assert(inst->src[i].stride == 0);
> inst->src[i] = byte_offset(
> retype(brw_reg, inst->src[i].type),
> - inst->src[i].subreg_offset);
> + inst->src[i].subnr);
>}
>}
> }
> @@ -1653,7 +1652,7 @@ fs_visitor::convert_attr_sources_to_hw_regs(fs_inst
*inst)
> unsigned width = inst->src[i].stride == 0 ? 1 : inst->exec_size;
> struct brw_reg reg =
> stride(byte_offset(retype(brw_vec8_grf(grf, 0), inst-
>src[i].type),
> - inst->src[i].subreg_offset),
> + inst->src[i].subnr),
> inst->exec_size * inst->src[i].stride,
> width, inst->src[i].stride);
> reg.abs = inst->src[i].abs;
> @@ -2597,7 +2596,7 @@ fs_visitor::compute_to_mrf()
> inst->dst.type != inst->src[0].type ||
> inst->src[0].abs || inst->src[0].negate ||
>!inst->src[0].is_contiguous() ||
> - inst->src[0].subreg_offset)
> + inst->src[0].subnr)
>continue;
>
>/* Work out which hardware MRF registers are written by this
> @@ -3367,7 +3366,7 @@ fs_visitor::lower_integer_multiplication()
> assert(src1_1_w.stride == 1);
> src1_1_w.stride = 2;
>}
> - src1_1_w.subreg_offset += type_sz(BRW_REGISTER_TYPE_UW);
> + src1_1_w.subnr += type_sz(BRW_REGISTER_TYPE_UW);
> }
> ibld.MUL(low, inst->src[0], src1_0_w);
> ibld.MUL(high, inst->src[0], src1_1_w);
> @@ -3386,7 +3385,7 @@ fs_visitor::lower_integer_multiplication()
>assert(src0_1_w.stride == 1);
>src0_1_w.stride = 2;
> }
> - src0_1_w.subreg_offset += type_sz(BRW_REGISTER_TYPE_UW);
> + src0_1_w.subnr += type_sz(BRW_REGISTER_TYPE_UW);
>
> ibld.MUL(low, src0_0_w, inst->src[1]);
> ibld.MUL(high, src0_1_w, inst->src[1]);
> @@ -3394,14 +3393,14 @@ fs_visitor::lower_integer_multiplication()
>
> fs_reg dst = inst->dst;
> dst.type = BRW_REGISTER_TYPE_UW;
> -dst.subreg_offset = 2;
> +dst.subnr = 2;
> dst.stride = 2;
>
> high.type = BRW_REGISTER_TYPE_UW;
> high.stride = 2;
>
> low.type = BRW_REGISTER_TYPE_UW;
> -low.subreg_offset = 2;
> +low.subnr = 2;
> low.stride = 2;
>
> ibld.ADD(dst, low, high);
> @@ -4642,9 +4641,9 @@ fs_visitor::dump_instruction(backend_instruction
*be_inst, FILE *file)
> case VGRF:
>fprintf(file, "vgrf%d", inst->dst.nr);
>if (alloc.sizes[inst->dst.nr] != inst->regs_written ||
> - inst->dst.subreg_offset)
> + inst->dst.subnr)
> fprintf(file, "+%d.%d",
> - inst->dst.reg_offset, inst->dst.subreg_offset);
> + inst->dst.reg_offset, inst->dst.subnr);
>break;
> case FIXED_GRF:
>fprintf(file, "g%d", inst->dst.nr);
> @@ -4698,9 +4697,9 @@ fs_visitor::dump_instruction(backend_instruction
*be_inst, FILE *file)
>case VGRF:
> fprintf(file, "vgrf%d", inst->src[i].nr);
> if (alloc.siz
[Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.
---
src/mesa/drivers/dri/i965/brw_fs.cpp | 31 +++---
.../drivers/dri/i965/brw_fs_combine_constants.cpp | 13 +
.../drivers/dri/i965/brw_fs_copy_propagation.cpp | 14 +-
src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 2 +-
src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 9 ++-
src/mesa/drivers/dri/i965/brw_ir_fs.h | 13 +++--
6 files changed, 35 insertions(+), 47 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
index 41a3f81..6ee590e 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -432,7 +432,6 @@ fs_reg::fs_reg(struct ::brw_reg reg) :
backend_reg(reg)
{
this->reg_offset = 0;
- this->subreg_offset = 0;
this->reladdr = NULL;
this->stride = 1;
if (this->file == IMM &&
@@ -447,7 +446,6 @@ bool
fs_reg::equals(const fs_reg &r) const
{
return (this->backend_reg::equals(r) &&
- subreg_offset == r.subreg_offset &&
!reladdr && !r.reladdr &&
stride == r.stride);
}
@@ -456,7 +454,8 @@ fs_reg &
fs_reg::set_smear(unsigned subreg)
{
assert(file != ARF && file != FIXED_GRF && file != IMM);
- subreg_offset = subreg * type_sz(type);
+ assert(subreg * type_sz(type) < (1 << 5)); /* subnr is 5 bits */
+ subnr = subreg * type_sz(type);
stride = 0;
return *this;
}
@@ -1513,7 +1512,7 @@ fs_visitor::assign_curb_setup()
assert(inst->src[i].stride == 0);
inst->src[i] = byte_offset(
retype(brw_reg, inst->src[i].type),
- inst->src[i].subreg_offset);
+ inst->src[i].subnr);
}
}
}
@@ -1653,7 +1652,7 @@ fs_visitor::convert_attr_sources_to_hw_regs(fs_inst *inst)
unsigned width = inst->src[i].stride == 0 ? 1 : inst->exec_size;
struct brw_reg reg =
stride(byte_offset(retype(brw_vec8_grf(grf, 0), inst->src[i].type),
- inst->src[i].subreg_offset),
+ inst->src[i].subnr),
inst->exec_size * inst->src[i].stride,
width, inst->src[i].stride);
reg.abs = inst->src[i].abs;
@@ -2597,7 +2596,7 @@ fs_visitor::compute_to_mrf()
inst->dst.type != inst->src[0].type ||
inst->src[0].abs || inst->src[0].negate ||
!inst->src[0].is_contiguous() ||
- inst->src[0].subreg_offset)
+ inst->src[0].subnr)
continue;
/* Work out which hardware MRF registers are written by this
@@ -3367,7 +3366,7 @@ fs_visitor::lower_integer_multiplication()
assert(src1_1_w.stride == 1);
src1_1_w.stride = 2;
}
- src1_1_w.subreg_offset += type_sz(BRW_REGISTER_TYPE_UW);
+ src1_1_w.subnr += type_sz(BRW_REGISTER_TYPE_UW);
}
ibld.MUL(low, inst->src[0], src1_0_w);
ibld.MUL(high, inst->src[0], src1_1_w);
@@ -3386,7 +3385,7 @@ fs_visitor::lower_integer_multiplication()
assert(src0_1_w.stride == 1);
src0_1_w.stride = 2;
}
- src0_1_w.subreg_offset += type_sz(BRW_REGISTER_TYPE_UW);
+ src0_1_w.subnr += type_sz(BRW_REGISTER_TYPE_UW);
ibld.MUL(low, src0_0_w, inst->src[1]);
ibld.MUL(high, src0_1_w, inst->src[1]);
@@ -3394,14 +3393,14 @@ fs_visitor::lower_integer_multiplication()
fs_reg dst = inst->dst;
dst.type = BRW_REGISTER_TYPE_UW;
-dst.subreg_offset = 2;
+dst.subnr = 2;
dst.stride = 2;
high.type = BRW_REGISTER_TYPE_UW;
high.stride = 2;
low.type = BRW_REGISTER_TYPE_UW;
-low.subreg_offset = 2;
+low.subnr = 2;
low.stride = 2;
ibld.ADD(dst, low, high);
@@ -4642,9 +4641,9 @@ fs_visitor::dump_instruction(backend_instruction
*be_inst, FILE *file)
case VGRF:
fprintf(file, "vgrf%d", inst->dst.nr);
if (alloc.sizes[inst->dst.nr] != inst->regs_written ||
- inst->dst.subreg_offset)
+ inst->dst.subnr)
fprintf(file, "+%d.%d",
- inst->dst.reg_offset, inst->dst.subreg_offset);
+ inst->dst.reg_offset, inst->dst.subnr);
break;
case FIXED_GRF:
fprintf(file, "g%d", inst->dst.nr);
@@ -4698,9 +4697,9 @@ fs_visitor::dump_instruction(backend_instruction
*be_inst, FILE *file)
case VGRF:
fprintf(file, "vgrf%d", inst->src[i].nr);
if (alloc.sizes[inst->src[i].nr] != (unsigned)inst->regs_read(i) ||
- inst->src[i].subreg_offset)
+ inst->src[i].subnr)
fprintf(file, "+%d.%d", inst->src[i].reg_offset,
-inst->src[i].subreg_offset);
+inst->src[i].subnr);
