[Mesa-dev] [PATCH v2 12/17] i965/vs: Rework vs_emit to take a nir_shader and a brw_compiler
Jason Ekstrand
jason at jlekstrand.net
Wed Oct 14 11:53:37 PDT 2015
On Wed, Oct 14, 2015 at 1:41 AM, Pohjolainen, Topi
<topi.pohjolainen at intel.com> wrote:
> On Wed, Oct 14, 2015 at 11:25:40AM +0300, Pohjolainen, Topi wrote:
>> On Sat, Oct 10, 2015 at 08:09:01AM -0700, Jason Ekstrand wrote:
>> > This commit removes all dependence on GL state by getting rid of the
>> > brw_context parameter and the GL data structures.
>> >
>> > v2 (Jason Ekstrand):
>> > - Patch use_legacy_snorm_formula through as a function argument rather
>> > than trying to go through the shader key.
>> > ---
>> > src/mesa/drivers/dri/i965/brw_vec4.cpp | 70 +++++++++++++---------------------
>> > src/mesa/drivers/dri/i965/brw_vs.c | 16 +++++++-
>> > src/mesa/drivers/dri/i965/brw_vs.h | 12 ++++--
>> > 3 files changed, 49 insertions(+), 49 deletions(-)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> > index 4b8390f..8e38729 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> > @@ -1937,51 +1937,42 @@ extern "C" {
>> > * Returns the final assembly and the program's size.
>> > */
>> > const unsigned *
>> > -brw_vs_emit(struct brw_context *brw,
>> > +brw_vs_emit(const struct brw_compiler *compiler, void *log_data,
>> > void *mem_ctx,
>> > const struct brw_vs_prog_key *key,
>> > struct brw_vs_prog_data *prog_data,
>> > - struct gl_vertex_program *vp,
>> > - struct gl_shader_program *prog,
>> > + const nir_shader *shader,
>> > + gl_clip_plane *clip_planes,
>> > + bool use_legacy_snorm_formula,
>> > int shader_time_index,
>> > - unsigned *final_assembly_size)
>> > + unsigned *final_assembly_size,
>> > + char **error_str)
>> > {
>> > const unsigned *assembly = NULL;
>> >
>> > - if (brw->intelScreen->compiler->scalar_vs) {
>> > + if (compiler->scalar_vs) {
>> > prog_data->base.dispatch_mode = DISPATCH_MODE_SIMD8;
>> >
>> > - fs_visitor v(brw->intelScreen->compiler, brw,
>> > - mem_ctx, key, &prog_data->base.base,
>> > + fs_visitor v(compiler, log_data, mem_ctx, key, &prog_data->base.base,
>> > NULL, /* prog; Only used for TEXTURE_RECTANGLE on gen < 8 */
>> > - vp->Base.nir, 8, shader_time_index);
>> > - if (!v.run_vs(brw_select_clip_planes(&brw->ctx))) {
>> > - if (prog) {
>> > - prog->LinkStatus = false;
>> > - ralloc_strcat(&prog->InfoLog, v.fail_msg);
>> > - }
>> > -
>> > - _mesa_problem(NULL, "Failed to compile vertex shader: %s\n",
>> > - v.fail_msg);
>> > + shader, 8, shader_time_index);
>> > + if (!v.run_vs(clip_planes)) {
>> > + if (error_str)
>> > + *error_str = ralloc_strdup(mem_ctx, v.fail_msg);
>>
>> I don't particularly like the complexity of the error reporting mechanism.
>> First vec4_visitor::fail() uses ralloc_asprintf() to create one string, then
>> we make a copy of it here and finally the caller of brw_vs_emit() makes yet
>> another copy using ralloc_strcat().
>> I wonder if we could pass the final destination all the way for the
>> vec4_visitor::fail() to augment with ralloc_asprintf() and hence avoid all
>
> Or more appropiately using ralloc_asprintf_append()...
>
>> the indirection in the middle. What do you think?
I'd be moderately ok with just doing "*error_str = v.fail_msg" and
avoiding the extra copy. I'm not a big fan of the extra copy, but I
decided to leave it in for a couple of reasons
1) It only happens on the error path so it's not a big deal.
2) Not copying it is kind of a layering violation. You're grabbing a
string from an object without copying it, destroying the object, and
then handing it back to the thing that called you. The only way this
works is if you know that the class ralloc'd the string from the
context you gave it. We do, in this case, but it did seem like a bit
of a layering violation.
3) The first time I did this rework, I created a new memory context
for *_emit and destroyed that memory context at the end. Because
fail_msg was allocated out of this temp context, I had to do something
with it before returning it. The objective there was to remove the
mem_ctx input parameter and make it more self-contained. Then I
realized that we had to do something about the pointer that gets
returned. The only sane use of the emit function is to call it,
immediately copy the result into some buffer, and destroy the context
so it wasn't like having a temp context just for *_emit was really
going help us keep stuff around for a shorter period of time. In the
end, I put the mem_ctx parameter back in.
There's my reasoning. That said, if you'd still like to get rid of
the copy, I'm ok with that.
--Jason
>> >
>> > return NULL;
>> > }
>> >
>> > - fs_generator g(brw->intelScreen->compiler, brw,
>> > - mem_ctx, (void *) key, &prog_data->base.base,
>> > - v.promoted_constants,
>> > + fs_generator g(compiler, log_data, mem_ctx, (void *) key,
>> > + &prog_data->base.base, v.promoted_constants,
>> > v.runtime_check_aads_emit, "VS");
>> > if (INTEL_DEBUG & DEBUG_VS) {
>> > - char *name;
>> > - if (prog) {
>> > - name = ralloc_asprintf(mem_ctx, "%s vertex shader %d",
>> > - prog->Label ? prog->Label : "unnamed",
>> > - prog->Name);
>> > - } else {
>> > - name = ralloc_asprintf(mem_ctx, "vertex program %d",
>> > - vp->Base.Id);
>> > - }
>> > - g.enable_debug(name);
>> > + const char *debug_name =
>> > + ralloc_asprintf(mem_ctx, "%s vertex shader %s",
>> > + shader->info.label ? shader->info.label : "unnamed",
>> > + shader->info.name);
>> > +
>> > + g.enable_debug(debug_name);
>> > }
>> > g.generate_code(v.cfg, 8);
>> > assembly = g.get_assembly(final_assembly_size);
>> > @@ -1990,26 +1981,19 @@ brw_vs_emit(struct brw_context *brw,
>> > if (!assembly) {
>> > prog_data->base.dispatch_mode = DISPATCH_MODE_4X2_DUAL_OBJECT;
>> >
>> > - vec4_vs_visitor v(brw->intelScreen->compiler, brw, key, prog_data,
>> > - vp->Base.nir, brw_select_clip_planes(&brw->ctx),
>> > - mem_ctx, shader_time_index,
>> > - !_mesa_is_gles3(&brw->ctx));
>> > + vec4_vs_visitor v(compiler, log_data, key, prog_data,
>> > + shader, clip_planes, mem_ctx,
>> > + shader_time_index, use_legacy_snorm_formula);
>> > if (!v.run()) {
>> > - if (prog) {
>> > - prog->LinkStatus = false;
>> > - ralloc_strcat(&prog->InfoLog, v.fail_msg);
>> > - }
>> > -
>> > - _mesa_problem(NULL, "Failed to compile vertex shader: %s\n",
>> > - v.fail_msg);
>> > + if (error_str)
>> > + *error_str = ralloc_strdup(mem_ctx, v.fail_msg);
>> >
>> > return NULL;
>> > }
>> >
>> > - vec4_generator g(brw->intelScreen->compiler, brw,
>> > - &prog_data->base,
>> > + vec4_generator g(compiler, log_data, &prog_data->base,
>> > mem_ctx, INTEL_DEBUG & DEBUG_VS, "vertex", "VS");
>> > - assembly = g.generate_assembly(v.cfg, final_assembly_size, vp->Base.nir);
>> > + assembly = g.generate_assembly(v.cfg, final_assembly_size, shader);
>> > }
>> >
>> > return assembly;
>> > diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c
>> > index 38de98f..abf744c 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_vs.c
>> > +++ b/src/mesa/drivers/dri/i965/brw_vs.c
>> > @@ -31,6 +31,7 @@
>> >
>> >
>> > #include "main/compiler.h"
>> > +#include "main/context.h"
>> > #include "brw_context.h"
>> > #include "brw_vs.h"
>> > #include "brw_util.h"
>> > @@ -179,9 +180,20 @@ brw_codegen_vs_prog(struct brw_context *brw,
>> >
>> > /* Emit GEN4 code.
>> > */
>> > - program = brw_vs_emit(brw, mem_ctx, key, &prog_data,
>> > - &vp->program, prog, st_index, &program_size);
>> > + char *error_str;
>> > + program = brw_vs_emit(brw->intelScreen->compiler, brw, mem_ctx, key,
>> > + &prog_data, vp->program.Base.nir,
>> > + brw_select_clip_planes(&brw->ctx),
>> > + !_mesa_is_gles3(&brw->ctx),
>> > + st_index, &program_size, &error_str);
>> > if (program == NULL) {
>> > + if (prog) {
>> > + prog->LinkStatus = false;
>> > + ralloc_strcat(&prog->InfoLog, error_str);
>> > + }
>> > +
>> > + _mesa_problem(NULL, "Failed to compile vertex shader: %s\n", error_str);
>> > +
>> > ralloc_free(mem_ctx);
>> > return false;
>> > }
>> > diff --git a/src/mesa/drivers/dri/i965/brw_vs.h b/src/mesa/drivers/dri/i965/brw_vs.h
>> > index c927cac..b65dd3b 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_vs.h
>> > +++ b/src/mesa/drivers/dri/i965/brw_vs.h
>> > @@ -54,14 +54,18 @@
>> > extern "C" {
>> > #endif
>> >
>> > -const unsigned *brw_vs_emit(struct brw_context *brw,
>> > +struct nir_shader;
>> > +
>> > +const unsigned *brw_vs_emit(const struct brw_compiler *compiler, void *log_data,
>> > void *mem_ctx,
>> > const struct brw_vs_prog_key *key,
>> > struct brw_vs_prog_data *prog_data,
>> > - struct gl_vertex_program *vp,
>> > - struct gl_shader_program *shader_prog,
>> > + const struct nir_shader *shader,
>> > + gl_clip_plane *clip_planes,
>> > + bool use_legacy_snorm_formula,
>> > int shader_time_index,
>> > - unsigned *program_size);
>> > + unsigned *final_assembly_size,
>> > + char **error_str);
>> > void brw_vs_debug_recompile(struct brw_context *brw,
>> > struct gl_shader_program *prog,
>> > const struct brw_vs_prog_key *key);
>> > --
>> > 2.5.0.400.gff86faf
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list