On Monday, July 17, 2017 6:12:57 PM PDT Matt Turner wrote:
> On Mon, Jul 17, 2017 at 5:10 PM, Kenneth Graunke <kenn...@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 <kenn...@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
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
