On Tue, Nov 13, 2018 at 9:48 AM Karol Herbst <kher...@redhat.com> wrote:
> From: Rob Clark <robdcl...@gmail.com>
>
> For cl we can have structs with 8/16/32/64 bit scalar types (as well as,
> ofc, arrays/structs/etc), which are padded according to 'C' rules. So
> for lowering struct deref's we need to not just consider a field's size,
> but also it's alignment.
>
> Signed-off-by: Karol Herbst <kher...@redhat.com>
> ---
> src/compiler/nir/nir.h | 10 +++++++
> src/compiler/nir/nir_lower_io.c | 52 ++++++++++++++++++++++++---------
> 2 files changed, 49 insertions(+), 13 deletions(-)
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index c469e111b2c..11e3d18320a 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -2825,10 +2825,20 @@ typedef enum {
> */
> nir_lower_io_force_sample_interpolation = (1 << 1),
> } nir_lower_io_options;
> +typedef struct nir_memory_model {
> + int (*type_size)(const struct glsl_type *);
> + int (*type_align)(const struct glsl_type *);
> +} nir_memory_model;
>
I don't really like the name "memory model". In my mind, that implies a
lot more than just a scheme for laying out memory. Maybe nir_io_layout_cb
or nir_io_type_size_align_cb?
I made this comment to Karol on IRC but I did something similar but with a
different approach with glsl_get_natural_size_align. I think I like this
approach better. It's potentially a bit less efficient but it's way
simpler. We should convert the constant lowering code over to it so we can
be consistent.
> bool nir_lower_io(nir_shader *shader,
> nir_variable_mode modes,
> int (*type_size)(const struct glsl_type *),
> nir_lower_io_options);
> +// TEMP use different name to avoid fixing all the callers yet:
> +bool nir_lower_io2(nir_shader *shader,
> + nir_variable_mode modes,
> + const nir_memory_model *mm,
> + nir_lower_io_options);
> +
> nir_src *nir_get_io_offset_src(nir_intrinsic_instr *instr);
> nir_src *nir_get_io_vertex_index_src(nir_intrinsic_instr *instr);
>
> diff --git a/src/compiler/nir/nir_lower_io.c
> b/src/compiler/nir/nir_lower_io.c
> index 2a6c284de2b..292baf9e4fc 100644
> --- a/src/compiler/nir/nir_lower_io.c
> +++ b/src/compiler/nir/nir_lower_io.c
> @@ -38,7 +38,7 @@
> struct lower_io_state {
> void *dead_ctx;
> nir_builder builder;
> - int (*type_size)(const struct glsl_type *type);
> + const nir_memory_model *mm;
> nir_variable_mode modes;
> nir_lower_io_options options;
> };
> @@ -86,12 +86,26 @@ nir_is_per_vertex_io(const nir_variable *var,
> gl_shader_stage stage)
> return false;
> }
>
> +static int
> +default_type_align(const struct glsl_type *type)
> +{
> + return 1;
> +}
> +
> +static inline int
> +align(int value, int alignment)
> +{
> + return (value + alignment - 1) & ~(alignment - 1);
> +}
>
we have an ALIGN macro which should be accessible from here which does
exactly that.
> +
> static nir_ssa_def *
> get_io_offset(nir_deref_instr *deref, nir_ssa_def **vertex_index,
> struct lower_io_state *state, unsigned *component)
> {
> nir_builder *b = &state->builder;
> - int (*type_size)(const struct glsl_type *) = state->type_size;
> + int (*type_size)(const struct glsl_type *) = state->mm->type_size;
> + int (*type_align)(const struct glsl_type *) = state->mm->type_align ?
> + state->mm->type_align : default_type_align;
> nir_deref_path path;
> nir_deref_path_init(&path, deref, NULL);
>
> @@ -137,7 +151,10 @@ get_io_offset(nir_deref_instr *deref, nir_ssa_def
> **vertex_index,
>
> unsigned field_offset = 0;
> for (unsigned i = 0; i < (*p)->strct.index; i++) {
> - field_offset += type_size(glsl_get_struct_field(parent->type,
> i));
> + const struct glsl_type *field_type =
> + glsl_get_struct_field(parent->type, i);
> + field_offset = align(field_offset, type_align(field_type));
> + field_offset += type_size(field_type);
> }
> offset = nir_iadd(b, offset, nir_imm_int(b, field_offset));
> } else {
> @@ -207,7 +224,7 @@ lower_load(nir_intrinsic_instr *intrin, struct
> lower_io_state *state,
> nir_intrinsic_set_component(load, component);
>
> if (load->intrinsic == nir_intrinsic_load_uniform)
> - nir_intrinsic_set_range(load, state->type_size(var->type));
> + nir_intrinsic_set_range(load, state->mm->type_size(var->type));
>
> if (vertex_index) {
> load->src[0] = nir_src_for_ssa(vertex_index);
> @@ -488,10 +505,8 @@ nir_lower_io_block(nir_block *block,
> }
>
> static bool
> -nir_lower_io_impl(nir_function_impl *impl,
> - nir_variable_mode modes,
> - int (*type_size)(const struct glsl_type *),
> - nir_lower_io_options options)
> +nir_lower_io_impl(nir_function_impl *impl, nir_variable_mode modes,
> + const nir_memory_model *mm, nir_lower_io_options
> options)
> {
> struct lower_io_state state;
> bool progress = false;
> @@ -499,7 +514,7 @@ nir_lower_io_impl(nir_function_impl *impl,
> nir_builder_init(&state.builder, impl);
> state.dead_ctx = ralloc_context(NULL);
> state.modes = modes;
> - state.type_size = type_size;
> + state.mm = mm;
> state.options = options;
>
> nir_foreach_block(block, impl) {
> @@ -514,22 +529,33 @@ nir_lower_io_impl(nir_function_impl *impl,
> }
>
> bool
> -nir_lower_io(nir_shader *shader, nir_variable_mode modes,
> - int (*type_size)(const struct glsl_type *),
> - nir_lower_io_options options)
> +nir_lower_io2(nir_shader *shader, nir_variable_mode modes,
> + const nir_memory_model *mm, nir_lower_io_options options)
> {
> bool progress = false;
>
> nir_foreach_function(function, shader) {
> if (function->impl) {
> progress |= nir_lower_io_impl(function->impl, modes,
> - type_size, options);
> + mm, options);
> }
> }
>
> return progress;
> }
>
> +
> +bool
> +nir_lower_io(nir_shader *shader, nir_variable_mode modes,
> + int (*type_size)(const struct glsl_type *),
> + nir_lower_io_options options)
> +{
> + nir_memory_model mm = {
> + .type_size = type_size,
> + };
> + return nir_lower_io2(shader, modes, &mm, options);
> +}
> +
> /**
> * Return the offset source for a load/store intrinsic.
> */
> --
> 2.19.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
