On Sat, Mar 26, 2016 at 8:48 AM, Rob Clark <[email protected]> wrote:
> On Fri, Mar 25, 2016 at 7:12 PM, Jason Ekstrand <[email protected]> > wrote: > > --- > > src/compiler/nir/nir.h | 3 ++ > > src/compiler/nir/nir_lower_system_values.c | 74 > ++++++++++++++++++++++++++++-- > > 2 files changed, 74 insertions(+), 3 deletions(-) > > > > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > > index 907dc34..d46858e 100644 > > --- a/src/compiler/nir/nir.h > > +++ b/src/compiler/nir/nir.h > > @@ -1678,6 +1678,9 @@ typedef struct nir_shader_compiler_options { > > * are simulated by floats.) > > */ > > bool native_integers; > > + > > + /* Indicates that the driver only has zero-based vertex id */ > > + bool vertex_id_zero_based; > > btw, mind setting that to true in ir3_nir.c or reminding me when you > push this so that I can? (From quick look, I think it should be false > for vc4..) > Done. I also threw setting the flag for i965 into this patch. I was going to do that on its own, but in here seems reasonable. > > } nir_shader_compiler_options; > > > > typedef struct nir_shader_info { > > diff --git a/src/compiler/nir/nir_lower_system_values.c > b/src/compiler/nir/nir_lower_system_values.c > > index 2bd787d..c1cd139 100644 > > --- a/src/compiler/nir/nir_lower_system_values.c > > +++ b/src/compiler/nir/nir_lower_system_values.c > > @@ -55,9 +55,77 @@ convert_block(nir_block *block, void *void_state) > > > > b->cursor = nir_after_instr(&load_var->instr); > > > > - nir_intrinsic_op sysval_op = > > - nir_intrinsic_from_system_value(var->data.location); > > - nir_ssa_def *sysval = nir_load_system_value(b, sysval_op, 0); > > + nir_ssa_def *sysval; > > + switch (var->data.location) { > > + case SYSTEM_VALUE_GLOBAL_INVOCATION_ID: { > > + /* From the GLSL man page for gl_GlobalInvocationID: > > + * > > + * "The value of gl_GlobalInvocationID is equal to > > + * gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID" > > + */ > > + > > + nir_const_value local_size; > > + local_size.u32[0] = b->shader->info.cs.local_size[0]; > > + local_size.u32[1] = b->shader->info.cs.local_size[1]; > > + local_size.u32[2] = b->shader->info.cs.local_size[2]; > > + > > + nir_ssa_def *group_id = > > + nir_load_system_value(b, nir_intrinsic_load_work_group_id, > 0); > > + nir_ssa_def *local_id = > > + nir_load_system_value(b, > nir_intrinsic_load_local_invocation_id, 0); > > + > > + sysval = nir_iadd(b, nir_imul(b, group_id, > > + nir_build_imm(b, 3, > local_size)), > > + local_id); > > + break; > > + } > > + > > + case SYSTEM_VALUE_LOCAL_INVOCATION_INDEX: { > > + /* From the GLSL man page for gl_LocalInvocationIndex: > > + * > > + * ?The value of gl_LocalInvocationIndex is equal to > > looks like that ? was supposed to be a " > Yup. > > + * gl_LocalInvocationID.z * gl_WorkGroupSize.x * > > + * gl_WorkGroupSize.y + gl_LocalInvocationID.y * > > + * gl_WorkGroupSize.x + gl_LocalInvocationID.x" > > + */ > > + nir_ssa_def *local_id = > > + nir_load_system_value(b, > nir_intrinsic_load_local_invocation_id, 0); > > + > > + unsigned stride_y = b->shader->info.cs.local_size[0]; > > + unsigned stride_z = b->shader->info.cs.local_size[0] * > > + b->shader->info.cs.local_size[1]; > > + > > + sysval = nir_iadd(b, nir_imul(b, nir_channel(b, local_id, 2), > > + nir_imm_int(b, stride_z)), > > + nir_iadd(b, nir_imul(b, nir_channel(b, > local_id, 1), > > + nir_imm_int(b, > stride_y)), > > + nir_channel(b, local_id, 0))); > > so this does get big enough to be difficult to read and the > stride_y/stride_z confused me ;-) > > Not sure if something like this is an improvement: > > nir_ssa_def *size_x = nir_imm_int(b, > b->shader->info.cs.local_size[0]); > nir_ssa_def *size_y = nir_imm_int(b, > b->shader->info.cs.local_size[1]); > > /* gl_LocalInvocationID.z * gl_WorkGroupSize.x * > gl_WorkGroupSize.y */ > sysval = nir_imul(nir_imul(b, nir_channel(b, local_id, 2), > size_x), size_y); > /* + gl_LocalInvocationID.y * gl_WorkGroupSize.x */ > sysval = nir_iadd(b, sysval, nir_imul(b, nir_channel(b, > local_id, 1), size_x)); > /* + gl_LocalInvocationID.x */ > sysval = nir_iadd(b, sysval, nir_channel(b, local_id, 0)); > > either way, I guess you don't need to pre-multiply size.x and size.y, > const prop pass will handle that, and not doing so makes it easier to > map back to the formula from the man page. And if you make them > nir_ssa_def's instead of unsigned then you could make the "math" part > of it less busy. > I like that much better. I updated the patch accordingly (but without the extra comments). > anyways, with those small comments, > > Reviewed-by: Rob Clark <[email protected]> > Thanks! > > + break; > > + } > > + > > + case SYSTEM_VALUE_VERTEX_ID: > > + if (b->shader->options->vertex_id_zero_based) { > > + sysval = nir_iadd(b, > > + nir_load_system_value(b, > nir_intrinsic_load_vertex_id_zero_base, 0), > > + nir_load_system_value(b, nir_intrinsic_load_base_vertex, > 0)); > > + } else { > > + sysval = nir_load_system_value(b, > nir_intrinsic_load_vertex_id, 0); > > + } > > + break; > > + > > + case SYSTEM_VALUE_INSTANCE_INDEX: > > + sysval = nir_iadd(b, > > + nir_load_system_value(b, nir_intrinsic_load_instance_id, 0), > > + nir_load_system_value(b, nir_intrinsic_load_base_instance, > 0)); > > + break; > > + > > + default: { > > + nir_intrinsic_op sysval_op = > > + nir_intrinsic_from_system_value(var->data.location); > > + sysval = nir_load_system_value(b, sysval_op, 0); > > + break; > > + } /* default */ > > + } > > > > nir_ssa_def_rewrite_uses(&load_var->dest.ssa, > nir_src_for_ssa(sysval)); > > nir_instr_remove(&load_var->instr); > > -- > > 2.5.0.400.gff86faf > > > > _______________________________________________ > > mesa-dev mailing list > > [email protected] > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
