[Mesa-dev] [PATCH 10/20] nir: Add pass to scalarize read_invocation/read_first_invocation
Kenneth Graunke
kenneth at whitecape.org
Tue Jul 18 04:14:31 UTC 2017
On Monday, July 17, 2017 6:12:57 PM PDT Matt Turner wrote:
> On Mon, Jul 17, 2017 at 5:10 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > On Thursday, July 6, 2017 4:48:20 PM PDT Matt Turner wrote:
> >> i965 will want these to be scalar operations.
> >> ---
> >> src/compiler/Makefile.sources | 1 +
> >> src/compiler/nir/nir.h | 2 +-
> >> .../nir/nir_lower_read_invocation_to_scalar.c | 112 +++++++++++++++++++++
> >> 3 files changed, 114 insertions(+), 1 deletion(-)
> >> create mode 100644 src/compiler/nir/nir_lower_read_invocation_to_scalar.c
> >>
> >> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> >> index b0f1c14b87..95f64f7a91 100644
> >> --- a/src/compiler/Makefile.sources
> >> +++ b/src/compiler/Makefile.sources
> >> @@ -229,6 +229,7 @@ NIR_FILES = \
> >> nir/nir_lower_passthrough_edgeflags.c \
> >> nir/nir_lower_patch_vertices.c \
> >> nir/nir_lower_phis_to_scalar.c \
> >> + nir/nir_lower_read_invocation_to_scalar.c \
> >> nir/nir_lower_regs_to_ssa.c \
> >> nir/nir_lower_returns.c \
> >> nir/nir_lower_samplers.c \
> >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> >> index 401c41f155..3591048574 100644
> >> --- a/src/compiler/nir/nir.h
> >> +++ b/src/compiler/nir/nir.h
> >> @@ -2433,7 +2433,7 @@ bool nir_move_vec_src_uses_to_dest(nir_shader *shader);
> >> bool nir_lower_vec_to_movs(nir_shader *shader);
> >> bool nir_lower_alu_to_scalar(nir_shader *shader);
> >> bool nir_lower_load_const_to_scalar(nir_shader *shader);
> >> -
> >> +bool nir_lower_read_invocation_to_scalar(nir_shader *shader);
> >> bool nir_lower_phis_to_scalar(nir_shader *shader);
> >> void nir_lower_io_to_scalar(nir_shader *shader, nir_variable_mode mask);
> >>
> >> diff --git a/src/compiler/nir/nir_lower_read_invocation_to_scalar.c b/src/compiler/nir/nir_lower_read_invocation_to_scalar.c
> >> new file mode 100644
> >> index 0000000000..edac7f5271
> >> --- /dev/null
> >> +++ b/src/compiler/nir/nir_lower_read_invocation_to_scalar.c
> >> @@ -0,0 +1,112 @@
> >> +/*
> >> + * Copyright © 2017 Intel Corporation
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> + * copy of this software and associated documentation files (the "Software"),
> >> + * to deal in the Software without restriction, including without limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice (including the next
> >> + * paragraph) shall be included in all copies or substantial portions of the
> >> + * Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >> + * IN THE SOFTWARE.
> >> + */
> >> +
> >> +#include "nir.h"
> >> +#include "nir_builder.h"
> >> +
> >> +/** @file nir_lower_read_invocation_to_scalar.c
> >> + *
> >> + * Replaces nir_intrinsic_read_invocation/nir_intrinsic_read_first_invocation
> >> + * operations with num_components != 1 with individual per-channel operations.
> >> + */
> >> +
> >> +static void
> >> +lower_read_invocation_to_scalar(nir_builder *b, nir_intrinsic_instr *intrin)
> >> +{
> >> + b->cursor = nir_before_instr(&intrin->instr);
> >> +
> >> + nir_ssa_def *value = nir_ssa_for_src(b, intrin->src[0], intrin->num_components);
> >> + nir_ssa_def *reads[4];
> >> +
> >> + for (unsigned i = 0; i < intrin->num_components; i++) {
> >> + nir_intrinsic_instr *chan_intrin =
> >> + nir_intrinsic_instr_create(b->shader, intrin->intrinsic);
> >> + nir_ssa_dest_init(&chan_intrin->instr, &chan_intrin->dest,
> >> + 1, intrin->dest.ssa.bit_size, NULL);
> >> + chan_intrin->num_components = 1;
> >> +
> >> + /* value */
> >> + chan_intrin->src[0] = nir_src_for_ssa(nir_channel(b, value, i));
> >> + /* invocation */
> >> + if (intrin->intrinsic == nir_intrinsic_read_invocation)
> >> + chan_intrin->src[1] = intrin->src[1];
> >
> > I don't think copying like this is legal. You need to use:
> >
> > nir_src_copy(&chan_intrin->src[1], &intrin->src[1], chan_intrin);
> >
> > otherwise you can get into trouble with ralloc contexts and indirects.
> >
>
>
> On Mon, Jul 17, 2017 at 5:10 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > On Thursday, July 6, 2017 4:48:20 PM PDT Matt Turner wrote:
> >> i965 will want these to be scalar operations.
> >> ---
> >> src/compiler/Makefile.sources | 1 +
> >> src/compiler/nir/nir.h | 2 +-
> >> .../nir/nir_lower_read_invocation_to_scalar.c | 112 +++++++++++++++++++++
> >> 3 files changed, 114 insertions(+), 1 deletion(-)
> >> create mode 100644 src/compiler/nir/nir_lower_read_invocation_to_scalar.c
> >>
> >> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> >> index b0f1c14b87..95f64f7a91 100644
> >> --- a/src/compiler/Makefile.sources
> >> +++ b/src/compiler/Makefile.sources
> >> @@ -229,6 +229,7 @@ NIR_FILES = \
> >> nir/nir_lower_passthrough_edgeflags.c \
> >> nir/nir_lower_patch_vertices.c \
> >> nir/nir_lower_phis_to_scalar.c \
> >> + nir/nir_lower_read_invocation_to_scalar.c \
> >> nir/nir_lower_regs_to_ssa.c \
> >> nir/nir_lower_returns.c \
> >> nir/nir_lower_samplers.c \
> >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> >> index 401c41f155..3591048574 100644
> >> --- a/src/compiler/nir/nir.h
> >> +++ b/src/compiler/nir/nir.h
> >> @@ -2433,7 +2433,7 @@ bool nir_move_vec_src_uses_to_dest(nir_shader *shader);
> >> bool nir_lower_vec_to_movs(nir_shader *shader);
> >> bool nir_lower_alu_to_scalar(nir_shader *shader);
> >> bool nir_lower_load_const_to_scalar(nir_shader *shader);
> >> -
> >> +bool nir_lower_read_invocation_to_scalar(nir_shader *shader);
> >> bool nir_lower_phis_to_scalar(nir_shader *shader);
> >> void nir_lower_io_to_scalar(nir_shader *shader, nir_variable_mode mask);
> >>
> >> diff --git a/src/compiler/nir/nir_lower_read_invocation_to_scalar.c b/src/compiler/nir/nir_lower_read_invocation_to_scalar.c
> >> new file mode 100644
> >> index 0000000000..edac7f5271
> >> --- /dev/null
> >> +++ b/src/compiler/nir/nir_lower_read_invocation_to_scalar.c
> >> @@ -0,0 +1,112 @@
> >> +/*
> >> + * Copyright © 2017 Intel Corporation
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> + * copy of this software and associated documentation files (the "Software"),
> >> + * to deal in the Software without restriction, including without limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice (including the next
> >> + * paragraph) shall be included in all copies or substantial portions of the
> >> + * Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >> + * IN THE SOFTWARE.
> >> + */
> >> +
> >> +#include "nir.h"
> >> +#include "nir_builder.h"
> >> +
> >> +/** @file nir_lower_read_invocation_to_scalar.c
> >> + *
> >> + * Replaces nir_intrinsic_read_invocation/nir_intrinsic_read_first_invocation
> >> + * operations with num_components != 1 with individual per-channel operations.
> >> + */
> >> +
> >> +static void
> >> +lower_read_invocation_to_scalar(nir_builder *b, nir_intrinsic_instr *intrin)
> >> +{
> >> + b->cursor = nir_before_instr(&intrin->instr);
> >> +
> >> + nir_ssa_def *value = nir_ssa_for_src(b, intrin->src[0], intrin->num_components);
> >> + nir_ssa_def *reads[4];
> >> +
> >> + for (unsigned i = 0; i < intrin->num_components; i++) {
> >> + nir_intrinsic_instr *chan_intrin =
> >> + nir_intrinsic_instr_create(b->shader, intrin->intrinsic);
> >> + nir_ssa_dest_init(&chan_intrin->instr, &chan_intrin->dest,
> >> + 1, intrin->dest.ssa.bit_size, NULL);
> >> + chan_intrin->num_components = 1;
> >> +
> >> + /* value */
> >> + chan_intrin->src[0] = nir_src_for_ssa(nir_channel(b, value, i));
> >> + /* invocation */
> >> + if (intrin->intrinsic == nir_intrinsic_read_invocation)
> >> + chan_intrin->src[1] = intrin->src[1];
> >
> > I don't think copying like this is legal. You need to use:
> >
> > nir_src_copy(&chan_intrin->src[1], &intrin->src[1], chan_intrin);
> >
> > otherwise you can get into trouble with ralloc contexts and indirects.
>
> Oh! I copied this structure from nir_lower_io_to_scalar.c
>
> Is it similarly broken there, or is there something different about
> that code that makes it safe?
Yep, that looks like a bug to me - it's snuck into a couple of different
passes. I'm pretty sure it works out unless your source is an indirectly
addressed register, which is pretty rare...so I wouldn't be surprised if
we just never hit the bug.
I'll send a patch to fix up the other occurrances. Thanks for pointing
this out!
--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170717/be24ea9a/attachment.sig>
More information about the mesa-dev
mailing list