On Wed, Nov 21, 2018 at 4:53 PM Jason Ekstrand <ja...@jlekstrand.net> wrote:
>
> 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 guess it is the part of the memory-model that lower_io needs.. maybe
nir_io_memory_model? But I guess I'd rather leave the name the same
and when we need to add other things memory model related we just add
to that same struct and pass it wherever else it is needed. It seems
nice to only have one struct of call backs for all things memory model
related and re-use that everywhere that memory model needs to be
abstracted rather than a different one for each lowering pass that
needs to care.
failing that nir_io_layout_cb is fine.. nir_io_type_size_align_cb
seems a bit clunky.
(my $0.02)
BR,
-R
> 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
