[Mesa-dev] [RFCv3 01/11] gallium: refactor pipe_shader_state to support multiple IR's
Rob Clark
robdclark at gmail.com
Wed Mar 30 13:59:41 UTC 2016
On Wed, Mar 30, 2016 at 9:49 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
> 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:
hmm, well nir permits c99, which I thought some other parts of the
code did not.. could be just some of the compiler flags we use on
parts of the code are more restrictive than they need to be? I do
remember it was initially resulting in a bunch of compiler warnings
but I'll have to check again.
> 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.)
hmm, well w/ some fwd declarations (ie. 'struct nir_shader;', etc)
they don't have to be void ptrs.. maybe that would be a good idea (and
I do like it when the compiler can find stupid typos for me..)
that ofc doesn't stop people from reading the wrong field.. although
that sort of issue would be easy to debug by just changing
s/union/struct/..
BR,
-R
> 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