[Mesa-dev] [PATCH 02/13] tgsi: simplify shader properties in tgsi_shader_info
Marek Olšák
maraeo at gmail.com
Tue Sep 30 12:45:46 PDT 2014
On Tue, Sep 30, 2014 at 9:04 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 30.09.2014 18:46, schrieb Marek Olšák:
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> Use an array of properties indexed by TGSI_PROPERTY_* definitions.
>> ---
>> src/gallium/auxiliary/draw/draw_gs.c | 23 ++++-----
>> src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c | 15 +++---
>> src/gallium/auxiliary/tgsi/tgsi_scan.c | 59 ++++++++++--------------
>> src/gallium/auxiliary/tgsi/tgsi_scan.h | 6 +--
>> src/gallium/auxiliary/util/u_pstipple.c | 8 +---
>> src/gallium/drivers/llvmpipe/lp_state_fs.c | 10 +---
>> src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c | 24 +++-------
>> src/gallium/drivers/r300/r300_fs.c | 8 +---
>> src/gallium/drivers/radeonsi/si_shader.c | 53 +++++++--------------
>> 9 files changed, 70 insertions(+), 136 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_gs.c b/src/gallium/auxiliary/draw/draw_gs.c
>> index 878fcca..0c2f892 100644
>> --- a/src/gallium/auxiliary/draw/draw_gs.c
>> +++ b/src/gallium/auxiliary/draw/draw_gs.c
>> @@ -750,9 +750,6 @@ draw_create_geometry_shader(struct draw_context *draw,
>> tgsi_scan_shader(state->tokens, &gs->info);
>>
>> /* setup the defaults */
>> - gs->input_primitive = PIPE_PRIM_TRIANGLES;
>> - gs->output_primitive = PIPE_PRIM_TRIANGLE_STRIP;
>> - gs->max_output_vertices = 32;
>> gs->max_out_prims = 0;
>>
>> #ifdef HAVE_LLVM
>> @@ -768,17 +765,15 @@ draw_create_geometry_shader(struct draw_context *draw,
>> gs->vector_length = 1;
>> }
>>
>> - for (i = 0; i < gs->info.num_properties; ++i) {
>> - if (gs->info.properties[i].name ==
>> - TGSI_PROPERTY_GS_INPUT_PRIM)
>> - gs->input_primitive = gs->info.properties[i].data[0];
>> - else if (gs->info.properties[i].name ==
>> - TGSI_PROPERTY_GS_OUTPUT_PRIM)
>> - gs->output_primitive = gs->info.properties[i].data[0];
>> - else if (gs->info.properties[i].name ==
>> - TGSI_PROPERTY_GS_MAX_OUTPUT_VERTICES)
>> - gs->max_output_vertices = gs->info.properties[i].data[0];
>> - }
>> + gs->input_primitive =
>> + gs->info.properties[TGSI_PROPERTY_GS_INPUT_PRIM][0];
>> + gs->output_primitive =
>> + gs->info.properties[TGSI_PROPERTY_GS_OUTPUT_PRIM][0];
>> + gs->max_output_vertices =
>> + gs->info.properties[TGSI_PROPERTY_GS_MAX_OUTPUT_VERTICES][0];
>> + if (!gs->max_output_vertices)
>> + gs->max_output_vertices = 32;
>> +
>> /* Primitive boundary is bigger than max_output_vertices by one, because
>> * the specification says that the geometry shader should exit if the
>> * number of emitted vertices is bigger or equal to max_output_vertices and
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>> index c0bd7be..2d7f32d 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>> @@ -3855,8 +3855,8 @@ lp_build_tgsi_soa(struct gallivm_state *gallivm,
>> * were forgetting so we're using MAX_VERTEX_VARYING from
>> * that spec even though we could debug_assert if it's not
>> * set, but that's a lot uglier. */
>> - uint max_output_vertices = 32;
>> - uint i = 0;
>> + uint max_output_vertices;
>> +
>> /* inputs are always indirect with gs */
>> bld.indirect_files |= (1 << TGSI_FILE_INPUT);
>> bld.gs_iface = gs_iface;
>> @@ -3864,12 +3864,11 @@ lp_build_tgsi_soa(struct gallivm_state *gallivm,
>> bld.bld_base.op_actions[TGSI_OPCODE_EMIT].emit = emit_vertex;
>> bld.bld_base.op_actions[TGSI_OPCODE_ENDPRIM].emit = end_primitive;
>>
>> - for (i = 0; i < info->num_properties; ++i) {
>> - if (info->properties[i].name ==
>> - TGSI_PROPERTY_GS_MAX_OUTPUT_VERTICES) {
>> - max_output_vertices = info->properties[i].data[0];
>> - }
>> - }
>> + max_output_vertices =
>> + info->properties[TGSI_PROPERTY_GS_MAX_OUTPUT_VERTICES][0];
>> + if (!max_output_vertices)
>> + max_output_vertices = 32;
>> +
>> bld.max_output_vertices_vec =
>> lp_build_const_int_vec(gallivm, bld.bld_base.int_bld.type,
>> max_output_vertices);
>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_scan.c b/src/gallium/auxiliary/tgsi/tgsi_scan.c
>> index c71bb36..f9d1896 100644
>> --- a/src/gallium/auxiliary/tgsi/tgsi_scan.c
>> +++ b/src/gallium/auxiliary/tgsi/tgsi_scan.c
>> @@ -277,13 +277,11 @@ tgsi_scan_shader(const struct tgsi_token *tokens,
>> {
>> const struct tgsi_full_property *fullprop
>> = &parse.FullToken.FullProperty;
>> + unsigned name = fullprop->Property.PropertyName;
>>
>> - info->properties[info->num_properties].name =
>> - fullprop->Property.PropertyName;
>> - memcpy(info->properties[info->num_properties].data,
>> - fullprop->u, 8 * sizeof(unsigned));;
>> -
>> - ++info->num_properties;
>> + assert(name < Elements(info->properties));
>> + memcpy(info->properties[name],
>> + fullprop->u, 8 * sizeof(unsigned));
>> }
>> break;
>>
>> @@ -296,35 +294,26 @@ tgsi_scan_shader(const struct tgsi_token *tokens,
>> info->opcode_count[TGSI_OPCODE_KILL]);
>>
>> /* extract simple properties */
>> - for (i = 0; i < info->num_properties; ++i) {
>> - switch (info->properties[i].name) {
>> - case TGSI_PROPERTY_FS_COORD_ORIGIN:
>> - info->origin_lower_left = info->properties[i].data[0];
>> - break;
>> - case TGSI_PROPERTY_FS_COORD_PIXEL_CENTER:
>> - info->pixel_center_integer = info->properties[i].data[0];
>> - break;
>> - case TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS:
>> - info->color0_writes_all_cbufs = info->properties[i].data[0];
>> - break;
>> - case TGSI_PROPERTY_GS_INPUT_PRIM:
>> - /* The dimensions of the IN decleration in geometry shader have
>> - * to be deduced from the type of the input primitive.
>> - */
>> - if (procType == TGSI_PROCESSOR_GEOMETRY) {
>> - unsigned input_primitive = info->properties[i].data[0];
>> - int num_verts = u_vertices_per_prim(input_primitive);
>> - int j;
>> - info->file_count[TGSI_FILE_INPUT] = num_verts;
>> - info->file_max[TGSI_FILE_INPUT] =
>> - MAX2(info->file_max[TGSI_FILE_INPUT], num_verts - 1);
>> - for (j = 0; j < num_verts; ++j) {
>> - info->file_mask[TGSI_FILE_INPUT] |= (1 << j);
>> - }
>> - }
>> - break;
>> - default:
>> - ;
>> + info->origin_lower_left =
>> + info->properties[TGSI_PROPERTY_FS_COORD_ORIGIN][0];
>> + info->pixel_center_integer =
>> + info->properties[TGSI_PROPERTY_FS_COORD_PIXEL_CENTER][0];
>> + info->color0_writes_all_cbufs =
>> + info->properties[TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS][0];
>> +
>> + /* The dimensions of the IN decleration in geometry shader have
>> + * to be deduced from the type of the input primitive.
>> + */
>> + if (procType == TGSI_PROCESSOR_GEOMETRY) {
>> + unsigned input_primitive =
>> + info->properties[TGSI_PROPERTY_GS_INPUT_PRIM][0];
>> + int num_verts = u_vertices_per_prim(input_primitive);
>> + int j;
>> + info->file_count[TGSI_FILE_INPUT] = num_verts;
>> + info->file_max[TGSI_FILE_INPUT] =
>> + MAX2(info->file_max[TGSI_FILE_INPUT], num_verts - 1);
>> + for (j = 0; j < num_verts; ++j) {
>> + info->file_mask[TGSI_FILE_INPUT] |= (1 << j);
>> }
>> }
>>
>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_scan.h b/src/gallium/auxiliary/tgsi/tgsi_scan.h
>> index 1869b41..0d79e29 100644
>> --- a/src/gallium/auxiliary/tgsi/tgsi_scan.h
>> +++ b/src/gallium/auxiliary/tgsi/tgsi_scan.h
>> @@ -91,11 +91,7 @@ struct tgsi_shader_info
>> */
>> unsigned indirect_files;
>>
>> - struct {
>> - unsigned name;
>> - unsigned data[8];
>> - } properties[TGSI_PROPERTY_COUNT];
>> - uint num_properties;
>> + unsigned properties[TGSI_PROPERTY_COUNT][8]; /* index with TGSI_PROPERTY_ */
>> };
> I don't think there's any properties which have more than one value,
> thus maybe it would be a good idea to just nuke the [8] array (and
> assert if there's more than one data token when parsing)? The choice of
> 8 seems completely arbitrary to me in any case (the token itself has 8
> _bits_ for NrTokens hence could have way more than 8 dwords).
> tgsi_full_token though indeed has these 8 values, though I don't really
> see a need for it.
That can be done in a later patch. Too many patches already depend on this one.
>
>>
>> extern void
>> diff --git a/src/gallium/auxiliary/util/u_pstipple.c b/src/gallium/auxiliary/util/u_pstipple.c
>> index 4f2e702..ce1fe5d 100644
>> --- a/src/gallium/auxiliary/util/u_pstipple.c
>> +++ b/src/gallium/auxiliary/util/u_pstipple.c
>> @@ -407,7 +407,6 @@ util_pstipple_create_fragment_shader(struct pipe_context *pipe,
>> struct pipe_shader_state *new_fs;
>> struct pstip_transform_context transform;
>> const uint newLen = tgsi_num_tokens(fs->tokens) + NUM_NEW_TOKENS;
>> - unsigned i;
>>
>> new_fs = MALLOC(sizeof(*new_fs));
>> if (!new_fs)
>> @@ -433,11 +432,8 @@ util_pstipple_create_fragment_shader(struct pipe_context *pipe,
>>
>> tgsi_scan_shader(fs->tokens, &transform.info);
>>
>> - /* find fragment coordinate origin property */
>> - for (i = 0; i < transform.info.num_properties; i++) {
>> - if (transform.info.properties[i].name == TGSI_PROPERTY_FS_COORD_ORIGIN)
>> - transform.coordOrigin = transform.info.properties[i].data[0];
>> - }
>> + transform.coordOrigin =
>> + transform.info.properties[TGSI_PROPERTY_FS_COORD_ORIGIN][0];
>>
>> tgsi_transform_shader(fs->tokens,
>> (struct tgsi_token *) new_fs->tokens,
>> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c b/src/gallium/drivers/llvmpipe/lp_state_fs.c
>> index 0b74d15..349d85a 100644
>> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
>> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
>> @@ -2212,14 +2212,8 @@ generate_fragment(struct llvmpipe_context *lp,
>> }
>>
>> /* check if writes to cbuf[0] are to be copied to all cbufs */
>> - cbuf0_write_all = FALSE;
>> - for (i = 0;i < shader->info.base.num_properties; i++) {
>> - if (shader->info.base.properties[i].name ==
>> - TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS) {
>> - cbuf0_write_all = TRUE;
>> - break;
>> - }
>> - }
>> + cbuf0_write_all =
>> + shader->info.base.properties[TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS][0];
> I'd like to point out this doesn't seem to be entirely the same thing
> (it's also used the same in other drivers by the looks of it), since
> previously we did not actually check the value at all, merely the
> presence of the property indicated this to be true. I think though due
> to the way this stuff is emitted this is in the end indeed the same. So,
> as long as there's no piglit regressions, this looks ok to me.
All code in Mesa either sets 1 or doesn't set the property at all, so
the result is equivalent.
Marek
More information about the mesa-dev
mailing list