Jason Ekstrand <ja...@jlekstrand.net> writes: > On Fri, Jun 26, 2015 at 11:51 AM, Francisco Jerez <curroje...@riseup.net> > wrote: >> Jason Ekstrand <ja...@jlekstrand.net> writes: >> >>> On Fri, Jun 26, 2015 at 8:52 AM, Francisco Jerez <curroje...@riseup.net> >>> wrote: >>>> Jason Ekstrand <ja...@jlekstrand.net> writes: >>>> >>>>> Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com> >>>>> --- >>>>> 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 <curroje...@riseup.net> >>>> >>>>> 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
