On Wed, 2017-05-03 at 16:47 -0700, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes:
>
> > Reorder the uniforms to load first the dvec4-aligned variables
> > in the push constant buffer and then push the vec4-aligned ones.
> >
> > This fixes a bug were the dvec3/4 might be loaded one part on a GRF
> > and
> > the rest in next GRF, so the region parameters to read that could
> > break
> > the HW rules.
> >
> > v2:
> > - Fix broken logic.
> > - Add a comment to explain what should be needed to optimise the
> > usage
> > of the push constant buffer slots, as this patch does not pack the
> > uniforms.
> >
> > Signed-off-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com>
> > Cc: "17.1" <mesa-sta...@lists.freedesktop.org>
> > ---
> > src/intel/compiler/brw_vec4.cpp | 97
> > +++++++++++++++++++++++++++++++----------
> > 1 file changed, 74 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_vec4.cpp
> > b/src/intel/compiler/brw_vec4.cpp
> > index 0909ddb586..18bfd48fa1 100644
> > --- a/src/intel/compiler/brw_vec4.cpp
> > +++ b/src/intel/compiler/brw_vec4.cpp
> > @@ -583,16 +583,44 @@ vec4_visitor::split_uniform_registers()
> > }
> > }
> >
> > +/* This function returns the register number where we placed the
> > uniform */
> > +static int
> > +set_push_constant_loc(const int nr_uniforms, int
> > *new_uniform_count,
> > + const int src, const int size,
> > + int *new_loc, int *new_chan,
> > + int *new_chans_used)
> > +{
> > + int dst;
> > + /* Find the lowest place we can slot this uniform in. */
> > + for (dst = 0; dst < nr_uniforms; dst++) {
> > + if (new_chans_used[dst] + size <= 4)
> > + break;
> > + }
> > +
> > + assert(dst < nr_uniforms);
> > +
> > + new_loc[src] = dst;
> > + new_chan[src] = new_chans_used[dst];
> > + new_chans_used[dst] += size;
> > +
> > + *new_uniform_count = MAX2(*new_uniform_count, dst + 1);
> > + return dst;
> > +}
> > +
> > void
> > vec4_visitor::pack_uniform_registers()
> > {
> > uint8_t chans_used[this->uniforms];
> > int new_loc[this->uniforms];
> > int new_chan[this->uniforms];
> > + bool is_aligned_to_dvec4[this->uniforms];
> > + int new_chans_used[this->uniforms];
> >
> > memset(chans_used, 0, sizeof(chans_used));
> > memset(new_loc, 0, sizeof(new_loc));
> > memset(new_chan, 0, sizeof(new_chan));
> > + memset(new_chans_used, 0, sizeof(new_chans_used));
> > + memset(is_aligned_to_dvec4, 0, sizeof(is_aligned_to_dvec4));
> >
> > /* Find which uniform vectors are actually used by the
> > program. We
> > * expect unused vector elements when we've moved array access
> > out
> > @@ -631,10 +659,19 @@ vec4_visitor::pack_uniform_registers()
> >
> > unsigned channel = BRW_GET_SWZ(inst->src[i].swizzle,
> > c) + 1;
> > unsigned used = MAX2(chans_used[reg], channel *
> > channel_size);
> > - if (used <= 4)
> > - chans_used[reg] = used;
> > - else
> > - chans_used[reg + 1] = used - 4;
> > + /* FIXME: Marked all channels as used, so each uniform
> > will
> > + * fully use one or two vec4s. If we want to pack
> > them, we need
> > + * to, among other changes, set chans_used[reg] =
> > used;
> > + * chans_used[reg+1] = used - 4; and fix the swizzle
> > at the
> > + * end in order to set the proper location.
> > + */
> > + if (used <= 4) {
> > + chans_used[reg] = 4;
>
> Uhm... So this change prevents the uniform packing pass from
> actually
> packing anything? Might affect more applications negatively than
> broken
> FP64 would. Are you planning to send a v3 that fixes the issue
> without
> disabling the optimization?
Yes, I am planning to send a v3 of this patch with the optimization in-
place.
> May be worth holding this off until then.
> Even if that means it will miss the v17.1 release it will probably
> make
> it for the next bug-fix release.
>
OK, thanks!
Sam
> > + } else {
> > + is_aligned_to_dvec4[reg] = true;
> > + is_aligned_to_dvec4[reg + 1] = true;
> > + chans_used[reg + 1] = 4;
> > + }
> > }
> > }
> >
> > @@ -659,42 +696,56 @@ vec4_visitor::pack_uniform_registers()
> >
> > int new_uniform_count = 0;
> >
> > + /* As the uniforms are going to be reordered, take the data
> > from a temporary
> > + * copy of the original param[].
> > + */
> > + gl_constant_value **param = ralloc_array(NULL,
> > gl_constant_value*,
> > + stage_prog_data-
> > >nr_params);
> > + memcpy(param, stage_prog_data->param,
> > + sizeof(gl_constant_value*) * stage_prog_data-
> > >nr_params);
> > +
> > /* Now, figure out a packing of the live uniform vectors into
> > our
> > - * push constants.
> > + * push constants. Start with dvec{3,4} because they are
> > aligned to
> > + * dvec4 size (2 vec4).
> > */
> > for (int src = 0; src < uniforms; src++) {
> > int size = chans_used[src];
> >
> > - if (size == 0)
> > + if (size == 0 || !is_aligned_to_dvec4[src])
> > continue;
> >
> > - int dst;
> > - /* Find the lowest place we can slot this uniform in. */
> > - for (dst = 0; dst < src; dst++) {
> > - if (chans_used[dst] + size <= 4)
> > - break;
> > + int dst = set_push_constant_loc(uniforms,
> > &new_uniform_count,
> > + src, size, new_loc,
> > new_chan,
> > + new_chans_used);
> > + if (dst != src) {
> > + /* Move the references to the data */
> > + for (int j = 0; j < size; j++) {
> > + stage_prog_data->param[dst * 4 + new_chan[src] + j] =
> > + param[src * 4 + j];
> > + }
> > }
> > + }
> >
> > - if (src == dst) {
> > - new_loc[src] = dst;
> > - new_chan[src] = 0;
> > - } else {
> > - new_loc[src] = dst;
> > - new_chan[src] = chans_used[dst];
> > + /* Continue with the rest of data, which is aligned to vec4. */
> > + for (int src = 0; src < uniforms; src++) {
> > + int size = chans_used[src];
> > +
> > + if (size == 0 || is_aligned_to_dvec4[src])
> > + continue;
> >
> > + int dst = set_push_constant_loc(uniforms,
> > &new_uniform_count,
> > + src, size, new_loc,
> > new_chan,
> > + new_chans_used);
> > + if (dst != src) {
> > /* Move the references to the data */
> > for (int j = 0; j < size; j++) {
> > stage_prog_data->param[dst * 4 + new_chan[src] + j] =
> > - stage_prog_data->param[src * 4 + j];
> > + param[src * 4 + j];
> > }
> > -
> > - chans_used[dst] += size;
> > - chans_used[src] = 0;
> > }
> > -
> > - new_uniform_count = MAX2(new_uniform_count, dst + 1);
> > }
> >
> > + ralloc_free(param);
> > this->uniforms = new_uniform_count;
> >
> > /* Now, update the instructions for our repacked uniforms. */
> > --
> > 2.11.0
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
