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?
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
