[Mesa-dev] [PATCH v2 16/19] i965/fs: Use the builder dispatch_width for computing register offsets
Reviewed-by: Topi Pohjolainen --- 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 * type_sz(reg.type)); case UNIFORM: reg.reg_offset += delta; -- 2.4.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 16/19] i965/fs: Use the builder dispatch_width for computing register offsets
Jason Ekstrand writes: > Reviewed-by: Topi Pohjolainen > --- > 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: Reviewed-by: Francisco Jerez > type_sz(reg.type)); > case UNIFORM: >reg.reg_offset += delta; > -- > 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
Re: [Mesa-dev] [PATCH v2 16/19] i965/fs: Use the builder dispatch_width for computing register offsets
On Fri, Jun 26, 2015 at 8:52 AM, Francisco Jerez wrote: > Jason Ekstrand writes: > >> Reviewed-by: Topi Pohjolainen >> --- >> 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? --Jason > Reviewed-by: Francisco Jerez > >> type_sz(reg.type)); >> case UNIFORM: >>reg.reg_offset += delta; >> -- >> 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
Re: [Mesa-dev] [PATCH v2 16/19] i965/fs: Use the builder dispatch_width for computing register offsets
Jason Ekstrand writes: > On Fri, Jun 26, 2015 at 8:52 AM, Francisco Jerez > wrote: >> Jason Ekstrand writes: >> >>> Reviewed-by: Topi Pohjolainen >>> --- >>> 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. > --Jason > >> Reviewed-by: Francisco Jerez >> >>> type_sz(reg.type)); >>> case UNIFORM: >>>reg.reg_offset += delta; >>> -- >>> 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
Re: [Mesa-dev] [PATCH v2 16/19] i965/fs: Use the builder dispatch_width for computing register offsets
On Fri, Jun 26, 2015 at 11:51 AM, Francisco Jerez wrote: > Jason Ekstrand writes: > >> On Fri, Jun 26, 2015 at 8:52 AM, Francisco Jerez >> wrote: >>> Jason Ekstrand writes: >>> Reviewed-by: Topi Pohjolainen --- 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(). --Jason >> --Jason >> >>> Reviewed-by: Francisco Jerez >>> type_sz(reg.type)); case UNIFORM: reg.reg_offset += delta; -- 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
Re: [Mesa-dev] [PATCH v2 16/19] i965/fs: Use the builder dispatch_width for computing register offsets
Jason Ekstrand writes: > On Fri, Jun 26, 2015 at 11:51 AM, Francisco Jerez > wrote: >> Jason Ekstrand writes: >> >>> On Fri, Jun 26, 2015 at 8:52 AM, Francisco Jerez >>> wrote: Jason Ekstrand writes: > Reviewed-by: Topi Pohjolainen > --- > 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 > type_sz(reg.type)); > case UNIFORM: >reg.reg_offset += delta; > -- > 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
