[Mesa-dev] [PATCH 02/13] tgsi: simplify shader properties in tgsi_shader_info
Roland Scheidegger
sroland at vmware.com
Tue Sep 30 12:04:05 PDT 2014
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.
>
> 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.
>
> /* TODO: actually pick these based on the fs and color buffer
> * characteristics. */
> diff --git a/src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c b/src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c
> index 1f1fba2..774b14b 100644
> --- a/src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c
> +++ b/src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c
> @@ -1138,24 +1138,12 @@ _nvfx_fragprog_translate(uint16_t oclass, struct nv30_fragprog *fp)
> fpc->num_regs = 2;
> memset(fp->texcoord, 0xff, sizeof(fp->texcoord));
>
> - for (unsigned i = 0; i < fp->info.num_properties; ++i) {
> - switch (fp->info.properties[i].name) {
> - case TGSI_PROPERTY_FS_COORD_ORIGIN:
> - if (fp->info.properties[i].data[0])
> - fp->coord_conventions |= NV30_3D_COORD_CONVENTIONS_ORIGIN_INVERTED;
> - break;
> - case TGSI_PROPERTY_FS_COORD_PIXEL_CENTER:
> - if (fp->info.properties[i].data[0])
> - fp->coord_conventions |= NV30_3D_COORD_CONVENTIONS_CENTER_INTEGER;
> - break;
> - case TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS:
> - if (fp->info.properties[i].data[0])
> - fp->rt_enable |= NV30_3D_RT_ENABLE_MRT;
> - break;
> - default:
> - break;
> - }
> - }
> + if (fp->info.properties[TGSI_PROPERTY_FS_COORD_ORIGIN][0])
> + fp->coord_conventions |= NV30_3D_COORD_CONVENTIONS_ORIGIN_INVERTED;
> + if (fp->info.properties[TGSI_PROPERTY_FS_COORD_PIXEL_CENTER][0])
> + fp->coord_conventions |= NV30_3D_COORD_CONVENTIONS_CENTER_INTEGER;
> + if (fp->info.properties[TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS][0])
> + fp->rt_enable |= NV30_3D_RT_ENABLE_MRT;
>
> if (!nvfx_fragprog_prepare(fpc))
> goto out_err;
> diff --git a/src/gallium/drivers/r300/r300_fs.c b/src/gallium/drivers/r300/r300_fs.c
> index 6e1b4e4..ddf944a 100644
> --- a/src/gallium/drivers/r300/r300_fs.c
> +++ b/src/gallium/drivers/r300/r300_fs.c
> @@ -468,12 +468,8 @@ static void r300_translate_fragment_shader(
>
> find_output_registers(&compiler, shader);
>
> - shader->write_all = FALSE;
> - for (i = 0; i < shader->info.num_properties; i++) {
> - if (shader->info.properties[i].name == TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS) {
> - shader->write_all = TRUE;
> - }
> - }
> + shader->write_all =
> + shader->info.properties[TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS][0];
>
> if (compiler.Base.Debug & RC_DBG_LOG) {
> DBG(r300, DBG_FP, "r300: Initial fragment program\n");
> diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
> index 276ba81..5c3efd4 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -2866,52 +2866,33 @@ int si_shader_create(struct si_screen *sscreen, struct si_shader *shader)
> bld_base->emit_epilogue = si_llvm_emit_vs_epilogue;
> }
> break;
> - case TGSI_PROCESSOR_GEOMETRY: {
> - int i;
> -
> + case TGSI_PROCESSOR_GEOMETRY:
> si_shader_ctx.radeon_bld.load_input = declare_input_gs;
> bld_base->emit_fetch_funcs[TGSI_FILE_INPUT] = fetch_input_gs;
> bld_base->emit_epilogue = si_llvm_emit_gs_epilogue;
>
> - for (i = 0; i < sel->info.num_properties; i++) {
> - switch (sel->info.properties[i].name) {
> - case TGSI_PROPERTY_GS_INPUT_PRIM:
> - shader->gs_input_prim = sel->info.properties[i].data[0];
> - break;
> - case TGSI_PROPERTY_GS_OUTPUT_PRIM:
> - shader->gs_output_prim = sel->info.properties[i].data[0];
> - break;
> - case TGSI_PROPERTY_GS_MAX_OUTPUT_VERTICES:
> - shader->gs_max_out_vertices = sel->info.properties[i].data[0];
> - break;
> - }
> - }
> + shader->gs_input_prim =
> + sel->info.properties[TGSI_PROPERTY_GS_INPUT_PRIM][0];
> + shader->gs_output_prim =
> + sel->info.properties[TGSI_PROPERTY_GS_OUTPUT_PRIM][0];
> + shader->gs_max_out_vertices =
> + sel->info.properties[TGSI_PROPERTY_GS_MAX_OUTPUT_VERTICES][0];
> break;
> - }
> - case TGSI_PROCESSOR_FRAGMENT: {
> - int i;
> -
> + case TGSI_PROCESSOR_FRAGMENT:
> si_shader_ctx.radeon_bld.load_input = declare_input_fs;
> bld_base->emit_epilogue = si_llvm_emit_fs_epilogue;
>
> - for (i = 0; i < sel->info.num_properties; i++) {
> - switch (sel->info.properties[i].name) {
> - case TGSI_PROPERTY_FS_DEPTH_LAYOUT:
> - switch (sel->info.properties[i].data[0]) {
> - case TGSI_FS_DEPTH_LAYOUT_GREATER:
> - shader->db_shader_control |=
> - S_02880C_CONSERVATIVE_Z_EXPORT(V_02880C_EXPORT_GREATER_THAN_Z);
> - break;
> - case TGSI_FS_DEPTH_LAYOUT_LESS:
> - shader->db_shader_control |=
> - S_02880C_CONSERVATIVE_Z_EXPORT(V_02880C_EXPORT_LESS_THAN_Z);
> - break;
> - }
> - break;
> - }
> + switch (sel->info.properties[TGSI_PROPERTY_FS_DEPTH_LAYOUT][0]) {
> + case TGSI_FS_DEPTH_LAYOUT_GREATER:
> + shader->db_shader_control |=
> + S_02880C_CONSERVATIVE_Z_EXPORT(V_02880C_EXPORT_GREATER_THAN_Z);
> + break;
> + case TGSI_FS_DEPTH_LAYOUT_LESS:
> + shader->db_shader_control |=
> + S_02880C_CONSERVATIVE_Z_EXPORT(V_02880C_EXPORT_LESS_THAN_Z);
> + break;
> }
> break;
> - }
> default:
> assert(!"Unsupported shader type");
> return -1;
>
Otherwise looks ok to me.
Roland
More information about the mesa-dev
mailing list