<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Nov 13, 2018 at 9:48 AM Karol Herbst <<a href="mailto:kherbst@redhat.com">kherbst@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Rob Clark <<a href="mailto:robdclark@gmail.com" target="_blank">robdclark@gmail.com</a>> 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 <<a href="mailto:kherbst@redhat.com" target="_blank">kherbst@redhat.com</a>> --- 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; </blockquote><div> </div><div>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?</div><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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); +} </blockquote><div> </div><div>we have an ALIGN macro which should be accessible from here which does exactly that. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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; + <a href="http://state.mm" rel="noreferrer" target="_blank">state.mm</a> = 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 <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div>