On Dec 3, 2015 3:01 PM, "Eric Anholt" <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> wrote: > > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > > > There is some special-casing needed in a competent back-end. However, they > > can do their special-casing easily enough based on whether or not the > > offset is a constant. In the mean time, having the *_indirect variants > > adds special cases a number of places where they don't need to be and, in > > general, only complicates things. To complicate matters, NIR had no way to > > convdert an indirect load/store to a direct one in the case that the > > indirect was a constant so we would still not really get what the back-ends > > wanted. The best solution seems to be to get rid of the *_indirect > > variants entirely. > > I've been putting off debugging this series because NIR intrinsic > documentation has been bad at const_index[] versus src[] explanations > and I only ever get my code working through cargo culting. It looks > like you simplify things a lot, but I'm going to ask for some > clarification still. > > > --- > > src/glsl/nir/nir_intrinsics.h | 64 ++++++++++++++++----------------- > > src/glsl/nir/nir_lower_phis_to_scalar.c | 4 --- > > src/glsl/nir/nir_print.c | 19 +++++----- > > 3 files changed, 38 insertions(+), 49 deletions(-) > > > > diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h > > index b2565c5..0fa5a27 100644 > > --- a/src/glsl/nir/nir_intrinsics.h > > +++ b/src/glsl/nir/nir_intrinsics.h > > @@ -228,54 +228,50 @@ SYSTEM_VALUE(num_work_groups, 3, 0) > > SYSTEM_VALUE(helper_invocation, 1, 0) > > > > /* > > - * The format of the indices depends on the type of the load. For uniforms, > > - * the first index is the base address and the second index is an offset that > > - * should be added to the base address. (This way you can determine in the > > - * back-end which variable is being accessed even in an array.) For inputs, > > - * the one and only index corresponds to the attribute slot. UBO loads also > > - * have a single index which is the base address to load from. > > + * All load operations have a source specifying an offset which may or may > > + * not be constant. If the shader is still in SSA or partial SSA form, then > > + * determining whether or not the offset is constant is trivial. This is > > + * always the last source in the intrinsic. > > * > > - * UBO loads have a (possibly constant) source which is the UBO buffer index. > > - * For each type of load, the _indirect variant has one additional source > > - * (the second in the case of UBO's) that is the is an indirect to be added to > > - * the constant address or base offset to compute the final offset. > > + * Uniforms have a constant index that provides a secondary base offset that > > + * should be added to the offset from the source. This allows back-ends to > > + * determine which uniform variable is being accessed. > > For clarity, since we have things that are indices/offsets and might be > constants but appear in src[]: s/constant index/constant_index/. Will do. > > - * For vector backends, the address is in terms of one vec4, and so each array > > - * element is +4 scalar components from the previous array element. For scalar > > - * backends, the address is in terms of a single 4-byte float/int and arrays > > - * elements begin immediately after the previous array element. > > + * UBO and SSBO loads have a (possibly constant) source which is the UBO > > + * buffer index. The pervertex_input intrinsic has a source which specifies > > + * the (possibly constant) vertex id to load from. > > + * > > + * The exact address type depends on the lowering pass that generates the > > + * load/store intrinsics. Typically, this is vec4 units for things such as > > + * varying slots and float units for fragment shader inputs. UBO and SSBO > > + * offsets are always in bytes. > > */ > > > > #define LOAD(name, extra_srcs, indices, flags) \ > > - INTRINSIC(load_##name, extra_srcs, ARR(1), true, 0, 0, indices, flags) \ > > - INTRINSIC(load_##name##_indirect, extra_srcs + 1, ARR(1, 1), \ > > - true, 0, 0, indices, flags) > > + INTRINSIC(load_##name, extra_srcs + 1, ARR(1, 1), true, 0, 0, indices, flags) > > > > -LOAD(uniform, 0, 2, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER) > > Uniforms had two const_index entries before? I've only ever used one. Yeah, they did for a while. It never affected any tgsi users though so I didn't bother updating those drivers. > > -LOAD(ubo, 1, 1, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER) > > -LOAD(input, 0, 1, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER) > > -LOAD(per_vertex_input, 1, 1, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER) > > -LOAD(ssbo, 1, 1, NIR_INTRINSIC_CAN_ELIMINATE) > > -LOAD(output, 0, 1, NIR_INTRINSIC_CAN_ELIMINATE) > > -LOAD(per_vertex_output, 1, 1, NIR_INTRINSIC_CAN_ELIMINATE) > > +LOAD(uniform, 0, 1, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER) > > +LOAD(ubo, 1, 0, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER) > > +LOAD(input, 0, 0, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER) > > +LOAD(per_vertex_input, 1, 0, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER) > > +LOAD(ssbo, 1, 0, NIR_INTRINSIC_CAN_ELIMINATE) > > +LOAD(output, 0, 0, NIR_INTRINSIC_CAN_ELIMINATE) > > +LOAD(per_vertex_output, 1, 0, NIR_INTRINSIC_CAN_ELIMINATE) > > I think now that we don't have multiple variants being generated by > LOAD(), baking the extra_srcs + 1 into the LOAD() calls would clarify > things: uniforms have 1 src and 1 index, ssbos have 2 srcs and 0 > indices, and it's right there in the call. Right. Good point. > > /* > > - * Stores work the same way as loads, except now the first register input is > > - * the value or array to store and the optional second input is the indirect > > - * offset. SSBO stores are similar, but they accept an extra source for the > > - * block index and an extra index with the writemask to use. > > + * Stores work the same way as loads, except now the first source is the value > > + * to store and the second input is the indirect offset. SSBO stores are > > + * similar, but they accept an extra source for the block index and an extra > > + * index with the writemask to use. > > */ > > Apparently the per vertex index or SSBO block is slipped between the > "first" and "second" inputs? That's weird. Suggestion for clarity > below. > > > #define STORE(name, extra_srcs, extra_srcs_size, extra_indices, flags) \ > > - INTRINSIC(store_##name, 1 + extra_srcs, \ > > - ARR(0, extra_srcs_size, extra_srcs_size, extra_srcs_size), \ > > - false, 0, 0, 1 + extra_indices, flags) \ > > - INTRINSIC(store_##name##_indirect, 2 + extra_srcs, \ > > + INTRINSIC(store_##name, 2 + extra_srcs, \ > > ARR(0, 1, extra_srcs_size, extra_srcs_size), \ > > - false, 0, 0, 1 + extra_indices, flags) > > + false, 0, 0, extra_indices, flags) > > > > STORE(output, 0, 0, 0, 0) > > STORE(per_vertex_output, 1, 1, 0, 0) > > STORE(ssbo, 1, 1, 1, 0) > > I think instead of the long form comment about the sources, I'd rather > just have: > > /* src[] = { value, offset }. No const_index */ > STORE(output, 0, 0, 0, 0) /* src[]: offset, value */ > /* src[] = { value, vertex_index, offset }. No const_index */ > STORE(per_vertex_output, 1, 1, 0, 0) > /* src[] = { value, block, offset }. const_index[] = { write_mask } */ > STORE(ssbo, 1, 1, 1, 0) I like that. We should do the same for outputs.