[Mesa-dev] [PATCH 18/24] i965/fs: Don't mutate multi-component arguments in sampler payload set-up.
Jason Ekstrand
jason at jlekstrand.net
Fri May 27 19:42:35 UTC 2016
On Thu, May 26, 2016 at 8:46 PM, Francisco Jerez <currojerez at riseup.net>
wrote:
> The Gen5+ sampler message payload construction code steps through the
> coordinate and derivative components by induction like 'coordinate =
> offset(coordinate, bld, 1)', the problem is that while doing that it
> may step one past the end of the coordinate vector causing an
> assertion failure in offset() if it happens to be a (single component)
> immediate. Right now coordinates and derivatives are typically passed
> as actual registers but that will no longer be the case when we start
> propagating constants into logical messages.
>
> Instead express coordinate components in closed form like
> 'offset(coordinate, bld, i)' -- The end result seems slightly more
> readable that way and it allows passing the coordinate and derivative
> registers by const reference instead of by value, so it seems like a
> clean-up in its own right.
>
Thank you!!! This is way better than the way it was before.
> ---
> src/mesa/drivers/dri/i965/brw_fs.cpp | 62
> +++++++++++++++---------------------
> 1 file changed, 25 insertions(+), 37 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 31ebbe3..e05977b 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -4022,9 +4022,9 @@ lower_sampler_logical_send_gen4(const fs_builder
> &bld, fs_inst *inst, opcode op,
>
> static void
> lower_sampler_logical_send_gen5(const fs_builder &bld, fs_inst *inst,
> opcode op,
> - fs_reg coordinate,
> + const fs_reg &coordinate,
> const fs_reg &shadow_c,
> - fs_reg lod, fs_reg lod2,
> + const fs_reg &lod, const fs_reg &lod2,
> const fs_reg &sample_index,
> const fs_reg &surface,
> const fs_reg &sampler,
> @@ -4044,10 +4044,10 @@ lower_sampler_logical_send_gen5(const fs_builder
> &bld, fs_inst *inst, opcode op,
> message.nr--;
> }
>
> - for (unsigned i = 0; i < coord_components; i++) {
> - bld.MOV(retype(offset(msg_coords, bld, i), coordinate.type),
> coordinate);
> - coordinate = offset(coordinate, bld, 1);
> - }
> + for (unsigned i = 0; i < coord_components; i++)
> + bld.MOV(retype(offset(msg_coords, bld, i), coordinate.type),
> + offset(coordinate, bld, i));
> +
> fs_reg msg_end = offset(msg_coords, bld, coord_components);
> fs_reg msg_lod = offset(msg_coords, bld, 4);
>
> @@ -4076,12 +4076,10 @@ lower_sampler_logical_send_gen5(const fs_builder
> &bld, fs_inst *inst, opcode op,
> */
> msg_end = msg_lod;
> for (unsigned i = 0; i < grad_components; i++) {
> - bld.MOV(msg_end, lod);
> - lod = offset(lod, bld, 1);
> + bld.MOV(msg_end, offset(lod, bld, i));
> msg_end = offset(msg_end, bld, 1);
>
> - bld.MOV(msg_end, lod2);
> - lod2 = offset(lod2, bld, 1);
> + bld.MOV(msg_end, offset(lod2, bld, i));
> msg_end = offset(msg_end, bld, 1);
> }
> break;
> @@ -4131,14 +4129,14 @@ is_high_sampler(const struct brw_device_info
> *devinfo, const fs_reg &sampler)
>
> static void
> lower_sampler_logical_send_gen7(const fs_builder &bld, fs_inst *inst,
> opcode op,
> - fs_reg coordinate,
> + const fs_reg &coordinate,
> const fs_reg &shadow_c,
> - fs_reg lod, fs_reg lod2,
> + fs_reg lod, const fs_reg &lod2,
> const fs_reg &sample_index,
> const fs_reg &mcs,
> const fs_reg &surface,
> const fs_reg &sampler,
> - fs_reg offset_value,
> + const fs_reg &offset_value,
> unsigned coord_components,
> unsigned grad_components)
> {
> @@ -4210,21 +4208,15 @@ lower_sampler_logical_send_gen7(const fs_builder
> &bld, fs_inst *inst, opcode op,
> * [hdr], [ref], x, dPdx.x, dPdy.x, y, dPdx.y, dPdy.y, z, dPdx.z,
> dPdy.z
> */
> for (unsigned i = 0; i < coord_components; i++) {
> - bld.MOV(sources[length], coordinate);
> - coordinate = offset(coordinate, bld, 1);
> + bld.MOV(sources[length], offset(coordinate, bld, i));
> length++;
>
> /* For cube map array, the coordinate is (u,v,r,ai) but there are
> * only derivatives for (u, v, r).
> */
> if (i < grad_components) {
> - bld.MOV(sources[length], lod);
> - lod = offset(lod, bld, 1);
> - length++;
> -
> - bld.MOV(sources[length], lod2);
> - lod2 = offset(lod2, bld, 1);
> - length++;
> + bld.MOV(sources[length++], offset(lod, bld, i));
> + bld.MOV(sources[length++], offset(lod2, bld, i));
> }
> }
>
> @@ -4239,13 +4231,12 @@ lower_sampler_logical_send_gen7(const fs_builder
> &bld, fs_inst *inst, opcode op,
> * On Gen9 they are u, v, lod, r
> */
> bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D), coordinate);
> - coordinate = offset(coordinate, bld, 1);
> length++;
>
> if (devinfo->gen >= 9) {
> if (coord_components >= 2) {
> - bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D),
> coordinate);
> - coordinate = offset(coordinate, bld, 1);
> + bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D),
> + offset(coordinate, bld, 1));
> }
> length++;
> }
> @@ -4254,8 +4245,8 @@ lower_sampler_logical_send_gen7(const fs_builder
> &bld, fs_inst *inst, opcode op,
> length++;
>
> for (unsigned i = devinfo->gen >= 9 ? 2 : 1; i < coord_components;
> i++) {
> - bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D),
> coordinate);
> - coordinate = offset(coordinate, bld, 1);
> + bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D),
> + offset(coordinate, bld, i));
> length++;
> }
>
> @@ -4293,8 +4284,8 @@ lower_sampler_logical_send_gen7(const fs_builder
> &bld, fs_inst *inst, opcode op,
> * texture coordinates.
> */
> for (unsigned i = 0; i < coord_components; i++) {
> - bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D),
> coordinate);
> - coordinate = offset(coordinate, bld, 1);
> + bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D),
> + offset(coordinate, bld, i));
> length++;
> }
>
> @@ -4306,20 +4297,18 @@ lower_sampler_logical_send_gen7(const fs_builder
> &bld, fs_inst *inst, opcode op,
>
> /* More crazy intermixing */
> for (unsigned i = 0; i < 2; i++) { /* u, v */
> - bld.MOV(sources[length], coordinate);
> - coordinate = offset(coordinate, bld, 1);
> + bld.MOV(sources[length], offset(coordinate, bld, i));
> length++;
>
Might be good to build the length++ into the bld.MOV while you're here.
Same for the ones below. If you don't want the churn, I can write a
follow-on easily enough.
> }
>
> for (unsigned i = 0; i < 2; i++) { /* offu, offv */
> - bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D),
> offset_value);
> - offset_value = offset(offset_value, bld, 1);
> + bld.MOV(retype(sources[length], BRW_REGISTER_TYPE_D),
> + offset(offset_value, bld, i));
> length++;
> }
>
> if (coord_components == 3) { /* r if present */
> - bld.MOV(sources[length], coordinate);
> - coordinate = offset(coordinate, bld, 1);
> + bld.MOV(sources[length], offset(coordinate, bld, 2));
> length++;
> }
>
> @@ -4332,8 +4321,7 @@ lower_sampler_logical_send_gen7(const fs_builder
> &bld, fs_inst *inst, opcode op,
> /* Set up the coordinate (except for cases where it was done above) */
> if (!coordinate_done) {
> for (unsigned i = 0; i < coord_components; i++) {
> - bld.MOV(sources[length], coordinate);
> - coordinate = offset(coordinate, bld, 1);
> + bld.MOV(sources[length], offset(coordinate, bld, i));
> length++;
> }
> }
> --
> 2.7.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160527/262ecaae/attachment-0001.html>
More information about the mesa-dev
mailing list