[Mesa-dev] [RFCv3 01/11] gallium: refactor pipe_shader_state to support multiple IR's
Jose Fonseca
jfonseca at vmware.com
Wed Mar 30 13:49:39 UTC 2016
On 30/03/16 14:43, Rob Clark wrote:
> On Wed, Mar 30, 2016 at 9:32 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
>> On 31/01/16 20:16, Rob Clark wrote:
>>>
>>> The goal is to allow the pipe driver to request something other than
>>> TGSI, but detect whether what is getting is TGSI vs what it requested.
>>> The pipe drivers will always have to support TGSI (and convert that into
>>> whatever it is that they prefer), but in some cases we should be able to
>>> skip the TGSI intermediate step (such as glsl->nir vs glsl->tgsi->nir).
>>>
>>> I think pipe_compute_state should get similar treatment. Currently,
>>> afaict, it has one user and one consumer, which has allowed it to be
>>> sloppy wrt. supporting alternative IR's.
>>>
>>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>>> ---
>>> src/gallium/auxiliary/hud/hud_context.c | 9 ++++-----
>>> src/gallium/auxiliary/postprocess/pp_run.c | 3 +--
>>> src/gallium/auxiliary/tgsi/tgsi_ureg.c | 4 +---
>>> src/gallium/auxiliary/util/u_simple_shaders.c | 18 ++++++++++++------
>>> src/gallium/auxiliary/util/u_tests.c | 3 ++-
>>> src/gallium/include/pipe/p_defines.h | 12 ++++++++++--
>>> src/gallium/include/pipe/p_state.h | 26
>>> +++++++++++++++++++++++++-
>>> 7 files changed, 55 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/src/gallium/auxiliary/hud/hud_context.c
>>> b/src/gallium/auxiliary/hud/hud_context.c
>>> index 75afebe..5542342 100644
>>> --- a/src/gallium/auxiliary/hud/hud_context.c
>>> +++ b/src/gallium/auxiliary/hud/hud_context.c
>>> @@ -1218,7 +1218,7 @@ hud_create(struct pipe_context *pipe, struct
>>> cso_context *cso)
>>> };
>>>
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> + struct pipe_shader_state state;
>>>
>>> if (!tgsi_text_translate(fragment_shader_text, tokens,
>>> Elements(tokens))) {
>>> assert(0);
>>> @@ -1227,7 +1227,7 @@ hud_create(struct pipe_context *pipe, struct
>>> cso_context *cso)
>>> FREE(hud);
>>> return NULL;
>>> }
>>> -
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> hud->fs_text = pipe->create_fs_state(pipe, &state);
>>> }
>>>
>>> @@ -1265,8 +1265,7 @@ hud_create(struct pipe_context *pipe, struct
>>> cso_context *cso)
>>> };
>>>
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> -
>>> + struct pipe_shader_state state;
>>> if (!tgsi_text_translate(vertex_shader_text, tokens,
>>> Elements(tokens))) {
>>> assert(0);
>>> pipe_resource_reference(&hud->font.texture, NULL);
>>> @@ -1274,7 +1273,7 @@ hud_create(struct pipe_context *pipe, struct
>>> cso_context *cso)
>>> FREE(hud);
>>> return NULL;
>>> }
>>> -
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> hud->vs = pipe->create_vs_state(pipe, &state);
>>> }
>>>
>>> diff --git a/src/gallium/auxiliary/postprocess/pp_run.c
>>> b/src/gallium/auxiliary/postprocess/pp_run.c
>>> index c6c7b88..75f398e 100644
>>> --- a/src/gallium/auxiliary/postprocess/pp_run.c
>>> +++ b/src/gallium/auxiliary/postprocess/pp_run.c
>>> @@ -272,8 +272,7 @@ pp_tgsi_to_state(struct pipe_context *pipe, const char
>>> *text, bool isvs,
>>> return NULL;
>>> }
>>>
>>> - state.tokens = tokens;
>>> - memset(&state.stream_output, 0, sizeof(state.stream_output));
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>>
>>> if (isvs) {
>>> ret_state = pipe->create_vs_state(pipe, &state);
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_ureg.c
>>> b/src/gallium/auxiliary/tgsi/tgsi_ureg.c
>>> index d681150..ee16f5d 100644
>>> --- a/src/gallium/auxiliary/tgsi/tgsi_ureg.c
>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_ureg.c
>>> @@ -1967,14 +1967,12 @@ void *ureg_create_shader( struct ureg_program
>>> *ureg,
>>> {
>>> struct pipe_shader_state state;
>>>
>>> - state.tokens = ureg_finalize(ureg);
>>> + pipe_shader_state_from_tgsi(&state, ureg_finalize(ureg));
>>> if(!state.tokens)
>>> return NULL;
>>>
>>> if (so)
>>> state.stream_output = *so;
>>> - else
>>> - memset(&state.stream_output, 0, sizeof(state.stream_output));
>>>
>>> switch (ureg->processor) {
>>> case TGSI_PROCESSOR_VERTEX:
>>> diff --git a/src/gallium/auxiliary/util/u_simple_shaders.c
>>> b/src/gallium/auxiliary/util/u_simple_shaders.c
>>> index 7ffb271..03a0f80 100644
>>> --- a/src/gallium/auxiliary/util/u_simple_shaders.c
>>> +++ b/src/gallium/auxiliary/util/u_simple_shaders.c
>>> @@ -121,12 +121,13 @@ void *util_make_layered_clear_vertex_shader(struct
>>> pipe_context *pipe)
>>> "MOV OUT[2], SV[0]\n"
>>> "END\n";
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> + struct pipe_shader_state state;
>>>
>>> if (!tgsi_text_translate(text, tokens, Elements(tokens))) {
>>> assert(0);
>>> return NULL;
>>> }
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> return pipe->create_vs_state(pipe, &state);
>>> }
>>>
>>> @@ -149,12 +150,13 @@ void
>>> *util_make_layered_clear_helper_vertex_shader(struct pipe_context *pipe)
>>> "MOV OUT[2].x, SV[0].xxxx\n"
>>> "END\n";
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> + struct pipe_shader_state state;
>>>
>>> if (!tgsi_text_translate(text, tokens, Elements(tokens))) {
>>> assert(0);
>>> return NULL;
>>> }
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> return pipe->create_vs_state(pipe, &state);
>>> }
>>>
>>> @@ -192,12 +194,13 @@ void *util_make_layered_clear_geometry_shader(struct
>>> pipe_context *pipe)
>>> "EMIT IMM[0].xxxx\n"
>>> "END\n";
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> + struct pipe_shader_state state;
>>>
>>> if (!tgsi_text_translate(text, tokens, Elements(tokens))) {
>>> assert(0);
>>> return NULL;
>>> }
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> return pipe->create_gs_state(pipe, &state);
>>> }
>>>
>>> @@ -471,7 +474,7 @@ util_make_fragment_passthrough_shader(struct
>>> pipe_context *pipe,
>>>
>>> char text[sizeof(shader_templ)+100];
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> + struct pipe_shader_state state;
>>>
>>> sprintf(text, shader_templ,
>>> write_all_cbufs ? "PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1\n" :
>>> "",
>>> @@ -482,6 +485,7 @@ util_make_fragment_passthrough_shader(struct
>>> pipe_context *pipe,
>>> assert(0);
>>> return NULL;
>>> }
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> #if 0
>>> tgsi_dump(state.tokens, 0);
>>> #endif
>>> @@ -558,7 +562,7 @@ util_make_fs_blit_msaa_gen(struct pipe_context *pipe,
>>> const char *type = tgsi_texture_names[tgsi_tex];
>>> char text[sizeof(shader_templ)+100];
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> + struct pipe_shader_state state;
>>>
>>> assert(tgsi_tex == TGSI_TEXTURE_2D_MSAA ||
>>> tgsi_tex == TGSI_TEXTURE_2D_ARRAY_MSAA);
>>> @@ -571,6 +575,7 @@ util_make_fs_blit_msaa_gen(struct pipe_context *pipe,
>>> assert(0);
>>> return NULL;
>>> }
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> #if 0
>>> tgsi_dump(state.tokens, 0);
>>> #endif
>>> @@ -658,7 +663,7 @@ util_make_fs_blit_msaa_depthstencil(struct
>>> pipe_context *pipe,
>>> const char *type = tgsi_texture_names[tgsi_tex];
>>> char text[sizeof(shader_templ)+100];
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> + struct pipe_shader_state state;
>>>
>>> assert(tgsi_tex == TGSI_TEXTURE_2D_MSAA ||
>>> tgsi_tex == TGSI_TEXTURE_2D_ARRAY_MSAA);
>>> @@ -669,6 +674,7 @@ util_make_fs_blit_msaa_depthstencil(struct
>>> pipe_context *pipe,
>>> assert(0);
>>> return NULL;
>>> }
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> #if 0
>>> tgsi_dump(state.tokens, 0);
>>> #endif
>>> diff --git a/src/gallium/auxiliary/util/u_tests.c
>>> b/src/gallium/auxiliary/util/u_tests.c
>>> index 006dfa9..9ee1a60 100644
>>> --- a/src/gallium/auxiliary/util/u_tests.c
>>> +++ b/src/gallium/auxiliary/util/u_tests.c
>>> @@ -422,13 +422,14 @@ null_constant_buffer(struct pipe_context *ctx)
>>> "MOV OUT[0], CONST[0]\n"
>>> "END\n";
>>> struct tgsi_token tokens[1000];
>>> - struct pipe_shader_state state = {tokens};
>>> + struct pipe_shader_state state;
>>>
>>> if (!tgsi_text_translate(text, tokens, Elements(tokens))) {
>>> puts("Can't compile a fragment shader.");
>>> util_report_result(FAIL);
>>> return;
>>> }
>>> + pipe_shader_state_from_tgsi(&state, tokens);
>>> fs = ctx->create_fs_state(ctx, &state);
>>> cso_set_fragment_shader_handle(cso, fs);
>>> }
>>> diff --git a/src/gallium/include/pipe/p_defines.h
>>> b/src/gallium/include/pipe/p_defines.h
>>> index 335f4e6..3a1c691 100644
>>> --- a/src/gallium/include/pipe/p_defines.h
>>> +++ b/src/gallium/include/pipe/p_defines.h
>>> @@ -717,12 +717,20 @@ enum pipe_shader_cap
>>>
>>> /**
>>> * Shader intermediate representation.
>>> + *
>>> + * Note that if the driver requests something other than TGSI, it must
>>> + * always be prepared to receive TGSI in addition to its preferred IR.
>>> + * If the driver requests TGSI as its preferred IR, it will *always*
>>> + * get TGSI.
>>> + *
>>> + * Note that PIPE_SHADER_IR_TGSI should be zero for backwards compat with
>>> + * state trackers that only understand TGSI.
>>> */
>>> enum pipe_shader_ir
>>> {
>>> - PIPE_SHADER_IR_TGSI,
>>> + PIPE_SHADER_IR_TGSI = 0,
>>> PIPE_SHADER_IR_LLVM,
>>> - PIPE_SHADER_IR_NATIVE
>>> + PIPE_SHADER_IR_NATIVE,
>>> };
>>>
>>> /**
>>> diff --git a/src/gallium/include/pipe/p_state.h
>>> b/src/gallium/include/pipe/p_state.h
>>> index 2e4d283..31e512c 100644
>>> --- a/src/gallium/include/pipe/p_state.h
>>> +++ b/src/gallium/include/pipe/p_state.h
>>> @@ -211,13 +211,37 @@ struct pipe_stream_output_info
>>> } output[PIPE_MAX_SO_OUTPUTS];
>>> };
>>>
>>> -
>>> +/**
>>> + * The 'type' parameter identifies whether the shader state contains TGSI
>>> + * tokens, etc. If the driver returns 'PIPE_SHADER_IR_TGSI' for the
>>> + * 'PIPE_SHADER_CAP_PREFERRED_IR' shader param, the ir will *always* be
>>> + * 'PIPE_SHADER_IR_TGSI' and the tokens ptr will be valid. If the driver
>>> + * requests a different 'pipe_shader_ir' type, then it must check the
>>> 'type'
>>> + * enum to see if it is getting TGSI tokens or its preferred IR.
>>> + *
>>> + * TODO pipe_compute_state should probably get similar treatment to
>>> handle
>>> + * multiple IR's in a cleaner way..
>>> + */
>>> struct pipe_shader_state
>>> {
>>> + enum pipe_shader_ir type;
>>> + /* TODO move tokens into union. */
>>> const struct tgsi_token *tokens;
>>> + union {
>>> + void *llvm;
>>> + void *native;
>>> + } ir;
>>
>>
>> You could just make this an anonymous union:
>>
>> struct pipe_shader_state
>> {
>> enum pipe_shader_ir type;
>> union {
>> const struct tgsi_token *tokens;
>> void *llvm;
>> void *native;
>> };
>>
>> I think this makes sense at least for a transitory period.
>>
>
> fwiw, I did start out w/ an anon union, but some compilers seemed
> unhappy about that
svga headers have anonymous union (see
src/gallium/drivers/svga/include/svga3d_types.h . GCC/Clang/MSVC handle
it fine.
NIR has too, see src/compiler/nir/nir.h:
typedef struct nir_src {
union {
nir_instr *parent_instr;
struct nir_if *parent_if;
};
struct list_head use_link;
union {
nir_reg_src reg;
nir_ssa_def *ssa;
};
bool is_ssa;
} nir_src;
So I don't think there's any compiler Mesa cares about that doesn't
support them.
> , so I moved to a named union (leaving tgsi ptr in
> place for now.. was planning to fix that up once patchset is merged,
> otherwise moving the tgsi ptr would be rebase hell)
>
>> Long term, I'm not sure an unions makes sense at all, especially when
>> llvm/native etc are typeless. That is, if we're not using the union to help
>> with type checking/casts, then we might as well just use one opaque void *
>> pointer for all:
>>
>> struct pipe_shader_state
>> {
>> enum pipe_shader_ir type;
>> void *ir;
>> ...
>>
>> and do the casts ourselves.
>
> I guess I can't think of a strong arg against an opaque ptr, other
> than any sort of tool that can show you references to a struct
> member[1], having separate ptrs in a union makes it easier to track
> down just the nir users, or just the llvm users, or so on..
Fair point. But it's misleading if all members are void pointer
(anybody could read from the wrong.)
I think it makes sense for drivers to have asserts on the type, and
check for PIPE_SHADER_IR_ refs.
Jose
More information about the mesa-dev
mailing list