On Dec 3, 2015 3:01 PM, "Eric Anholt" <e...@anholt.net> wrote:
>
> Jason Ekstrand <ja...@jlekstrand.net> 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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
