I forgot to add an important piece of info. I also had to add this in
the opt_dead_code.cpp, do_dead_code():
if (strcmp(entry->var->name, "stream") == 0)
continue;
without that, the variable referenced by ir_emit_vertex() gets trashed
anyway, whether the ralloc context in link_shaders is detroyed or not.
Iago
On Fri, 2014-06-13 at 10:09 +0200, Iago Toral wrote:
> After debugging I have more information about what is going on. There
> are two problems, one is that the stream variable in ir_emit_vertex gets
> trashed and the other one is that even if we manage to avoid that it
> won't get its value assigned. I explain how these two come to happen
> below and maybe someone can point me to what I am doing wrong:
>
> first, this is how I am defining EmitStreamVertex():
>
> ir_function_signature *
> builtin_builder::_EmitStreamVertex(builtin_available_predicate avail,
> const glsl_type *stream_type)
> {
> ir_variable *stream =
> new(mem_ctx) ir_variable(stream_type, "stream", ir_var_const_in);
> MAKE_SIG(glsl_type::void_type, avail, 1, stream);
> ir_emit_vertex *ir = new(mem_ctx) ir_emit_vertex();
> ir->stream = var_ref(stream);
> body.emit(ir);
> return sig;
> }
>
> The pattern is similar to other built-in functions. Notice how
> ir_stream_vertex will take a reference to the input stream variable.
>
> And this is how I am defining ir_emit_vertex:
>
> class ir_emit_vertex : public ir_instruction {
> public:
> ir_emit_vertex() : ir_instruction(ir_type_emit_vertex), stream(NULL) {}
>
> virtual void accept(ir_visitor *v) { v->visit(this); }
>
> virtual ir_emit_vertex *clone(void *mem_ctx, struct hash_table *ht)
> const
> {
> ir_emit_vertex *ir = new(mem_ctx) ir_emit_vertex();
> if (this->stream)
> ir->stream = this->stream->clone(mem_ctx, ht);
> return ir;
> }
>
> virtual ir_visitor_status accept(ir_hierarchical_visitor *);
> ir_dereference_variable *stream;
> };
>
> Again, I don't see anything special as it follows the same pattern as
> other IR definitions in ir.h.
>
> If I only do this, then, by the time I reach
> vec4_gs_visitor::visit(ir_emit_vertex *ir), ir->stream has been trashed.
>
> ¿Is this expected? ¿Am I missing something in EmitStreamVertex(),
> ir_emit_vertex or somewhere else that is causing this?
>
> Valgrind says the variable gets killed with the destruction of the
> ralloc context created in link_shaders. And indeed, removing the
> ralloc_free lets the variable reach the visitor. I suppose this is not
> expected (otherwise we would have this problem in any built-in function
> that accepts input parameters). Just in case, I noticed this code in
> link_shaders:
>
> clone_ir_list(mem_ctx, linked->ir, main->ir);
>
> It seems that it clones code using that ralloc context created in
> link_shaders, so I changed it to be:
>
> clone_ir_list(linked, linked->ir, main->ir);
>
> And it fixes the problem, but I suppose this is only a workaround for
> the real problem.
>
> As for the second problem, if I bypass the variable trashing by removing
> the call to ralloc_free in link_shaders() or by doing the change above,
> then when we reach vec4_gs_visitor::visit(ir_emit_vertex *ir), if I do
> ((ir_rvalue*)ir->stream)->as_constant() it will still return NULL, so it
> is useless. I want to read the value of the variable here, which I
> should be able to do since this is a constant expression.
>
> However, as far as I can see by looking into ir_call::generate_inline()
> this seems to be expected: inputs to functions get a *new* variable for
> them, where the actual parameter value is set via an ir_assignment:
>
> parameters[i] = sig_param->clone(ctx, ht);
> parameters[i]->data.mode = ir_var_auto;
> (...)
> assign =
> new(ctx) ir_assignment(new(ctx)
> ir_dereference_variable(parameters[i]),
> param, NULL);
> next_ir->insert_before(assign);
> (...)
>
> And then it clones the body of the function, like so:
>
> foreach_list(n, &callee->body) {
> ir_instruction *ir = (ir_instruction *) n;
> ir_instruction *new_ir = ir->clone(ctx, ht);
>
> new_instructions.push_tail(new_ir);
> visit_tree(new_ir, replace_return_with_assignment,
> this->return_deref);
> }
>
> In our case, there is only one instruction here: ir_emit_vertex, and
> when cloning it we are also cloning its reference to the stream
> variable, but this is *different* from the parameter variable where we
> copied the actual parameter value, so there is no way we will be able to
> access the value of the variable from this reference.
>
> I can work around this problem in the visitor by doing this:
>
> dst_reg *reg = variable_storage(ir->stream->var);
>
> This seems to return the register associated with the real stream
> parameter that was the target of the ir_assignment, and then work with
> reg directly rather than creating a register in the visitor and moving
> the stream_id value to it. If I do it like this, however, I can't do
> some things that I was doing before, like not generating any code to set
> control data bits when we are calling EmitStreamVertex(0), because I
> can't access the value of the variable in the visitor to assess its
> value.
>
> So that's it, hopefully that's enough information for someone to tell
> what is missing in the implementation or if there is some other problem
> that is causing all this.
>
> Iago
>
> On Wed, 2014-06-11 at 21:25 +1200, Chris Forbes wrote:
> > This is pretty weird.
> >
> > We should be able to generate a normal builtin function body here
> > which consists of just the ir_emit_vertex, passing the stream
> > parameter to it. This would then get inlined like any other function
> > leaving a bare ir_emit_vertex / ir_end_primitive with a constant
> > operand. If one of the optimization passes eats that, then it's
> > broken.
> >
> >
> > On Wed, Jun 11, 2014 at 7:49 PM, Iago Toral Quiroga <ito...@igalia.com>
> > wrote:
> > > ---
> > > src/glsl/ast_function.cpp | 37 +++++++++++++++++++++-----
> > > src/glsl/builtin_functions.cpp | 60
> > > ++++++++++++++++++++++++++++++++++++++++++
> > > src/glsl/ir.h | 18 ++++++++-----
> > > 3 files changed, 103 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> > > index 8e91a1e..1c4d7e7 100644
> > > --- a/src/glsl/ast_function.cpp
> > > +++ b/src/glsl/ast_function.cpp
> > > @@ -1722,15 +1722,40 @@ ast_function_expression::hir(exec_list
> > > *instructions,
> > > ir_function_signature *sig =
> > > match_function_by_name(func_name, &actual_parameters, state);
> > >
> > > + bool is_emit_stream_vertex = !strcmp(func_name,
> > > "EmitStreamVertex");
> > > + bool is_end_stream_primitive = !strcmp(func_name,
> > > "EndStreamPrimitive");
> > > +
> > > ir_rvalue *value = NULL;
> > > if (sig == NULL) {
> > > - no_matching_function_error(func_name, &loc, &actual_parameters,
> > > state);
> > > - value = ir_rvalue::error_value(ctx);
> > > + no_matching_function_error(func_name, &loc, &actual_parameters,
> > > state);
> > > + value = ir_rvalue::error_value(ctx);
> > > } else if (!verify_parameter_modes(state, sig, actual_parameters,
> > > this->expressions)) {
> > > - /* an error has already been emitted */
> > > - value = ir_rvalue::error_value(ctx);
> > > - } else {
> > > - value = generate_call(instructions, sig, &actual_parameters,
> > > state);
> > > + /* an error has already been emitted */
> > > + value = ir_rvalue::error_value(ctx);
> > > + } else if (is_emit_stream_vertex || is_end_stream_primitive) {
> > > + /* See builtin_builder::_EmitStreamVertex() to undertand why we
> > > need
> > > + * to handle these as a special case.
> > > + */
> > > + ir_rvalue *stream_param = (ir_rvalue *) actual_parameters.head;
> > > + ir_constant *stream_const = stream_param->as_constant();
> > > + /* stream_const should not be NULL if we got here */
> > > + assert(stream_const);
> > > + int stream_id = stream_const->value.i[0];
> > > + if (stream_id < 0 || stream_id >= MAX_VERTEX_STREAMS) {
> > > + _mesa_glsl_error(&loc, state,
> > > + "Invalid call %s(%d). Accepted "
> > > + "values for the stream parameter are in the
> > > "
> > > + "range [0, %d].",
> > > + func_name, stream_id, MAX_VERTEX_STREAMS -
> > > 1);
> > > + }
> > > + /* Only enable multi-stream if we emit vertices to non-zero
> > > streams */
> > > + state->gs_uses_streams = state->gs_uses_streams || stream_id >
> > > 0;
> > > + if (is_emit_stream_vertex)
> > > + instructions->push_tail(new(ctx) ir_emit_vertex(stream_id));
> > > + else
> > > + instructions->push_tail(new(ctx)
> > > ir_end_primitive(stream_id));
> > > + } else {
> > > + value = generate_call(instructions, sig, &actual_parameters,
> > > state);
> > > }
> > >
> > > return value;
> > > diff --git a/src/glsl/builtin_functions.cpp
> > > b/src/glsl/builtin_functions.cpp
> > > index f9f0686..412e8f3 100644
> > > --- a/src/glsl/builtin_functions.cpp
> > > +++ b/src/glsl/builtin_functions.cpp
> > > @@ -359,6 +359,12 @@ shader_image_load_store(const _mesa_glsl_parse_state
> > > *state)
> > > state->ARB_shader_image_load_store_enable);
> > > }
> > >
> > > +static bool
> > > +gs_streams(const _mesa_glsl_parse_state *state)
> > > +{
> > > + return gpu_shader5(state) && gs_only(state);
> > > +}
> > > +
> > > /** @} */
> > >
> > >
> > > /******************************************************************************/
> > > @@ -594,6 +600,10 @@ private:
> > >
> > > B0(EmitVertex)
> > > B0(EndPrimitive)
> > > + ir_function_signature *_EmitStreamVertex(builtin_available_predicate
> > > avail,
> > > + const glsl_type
> > > *stream_type);
> > > + ir_function_signature
> > > *_EndStreamPrimitive(builtin_available_predicate avail,
> > > + const glsl_type
> > > *stream_type);
> > >
> > > B2(textureQueryLod);
> > > B1(textureQueryLevels);
> > > @@ -1708,6 +1718,14 @@ builtin_builder::create_builtins()
> > >
> > > add_function("EmitVertex", _EmitVertex(), NULL);
> > > add_function("EndPrimitive", _EndPrimitive(), NULL);
> > > + add_function("EmitStreamVertex",
> > > + _EmitStreamVertex(gs_streams, glsl_type::uint_type),
> > > + _EmitStreamVertex(gs_streams, glsl_type::int_type),
> > > + NULL);
> > > + add_function("EndStreamPrimitive",
> > > + _EndStreamPrimitive(gs_streams, glsl_type::uint_type),
> > > + _EndStreamPrimitive(gs_streams, glsl_type::int_type),
> > > + NULL);
> > >
> > > add_function("textureQueryLOD",
> > > _textureQueryLod(glsl_type::sampler1D_type,
> > > glsl_type::float_type),
> > > @@ -3878,6 +3896,35 @@ builtin_builder::_EmitVertex()
> > > }
> > >
> > > ir_function_signature *
> > > +builtin_builder::_EmitStreamVertex(builtin_available_predicate avail,
> > > + const glsl_type *stream_type)
> > > +{
> > > + ir_variable *stream =
> > > + new(mem_ctx) ir_variable(stream_type, "stream", ir_var_const_in);
> > > +
> > > + /* EmitStreamVertex is a special kind of built-in function. It does
> > > + * not really generate any IR when processed, instead, it only adds a
> > > + * marker in the IR to know when we need to generate code for vertex
> > > + * emission, just like EmitVertex(). However, in this case we have
> > > + * an input parameter that we need to preserve and that the dead code
> > > + * optimization passes will kill because it is apparently unused.
> > > + * We will actually use it, but not until code generation time, after
> > > + * the dead code elimination passes have run and kill the input
> > > variable.
> > > + *
> > > + * To deal with this situation, since the input parameter for
> > > + * EmitStreamVertex() must be a constant expression, we don't
> > > generate a
> > > + * function body here. Then, when we detect EmitVertexStream() calls
> > > the
> > > + * AST, instead of producing an ir_call, we get the constant value of
> > > + * the input parameter in that moment and produce a ir_emit_vertex
> > > directly.
> > > + * See ast_function_expression::hir().
> > > + */
> > > +
> > > + MAKE_INTRINSIC(glsl_type::void_type, avail, 1, stream);
> > > +
> > > + return sig;
> > > +}
> > > +
> > > +ir_function_signature *
> > > builtin_builder::_EndPrimitive()
> > > {
> > > MAKE_SIG(glsl_type::void_type, gs_only, 0);
> > > @@ -3888,6 +3935,19 @@ builtin_builder::_EndPrimitive()
> > > }
> > >
> > > ir_function_signature *
> > > +builtin_builder::_EndStreamPrimitive(builtin_available_predicate avail,
> > > + const glsl_type *stream_type)
> > > +{
> > > + ir_variable *stream =
> > > + new(mem_ctx) ir_variable(stream_type, "stream", ir_var_const_in);
> > > +
> > > + /* See comment in builtin_builder::_EmitStreamVertex, same applies
> > > here */
> > > + MAKE_INTRINSIC(glsl_type::void_type, avail, 1, stream);
> > > +
> > > + return sig;
> > > +}
> > > +
> > > +ir_function_signature *
> > > builtin_builder::_textureQueryLod(const glsl_type *sampler_type,
> > > const glsl_type *coord_type)
> > > {
> > > diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> > > index 8cc58af..7ae91ab 100644
> > > --- a/src/glsl/ir.h
> > > +++ b/src/glsl/ir.h
> > > @@ -2159,8 +2159,9 @@ private:
> > > */
> > > class ir_emit_vertex : public ir_instruction {
> > > public:
> > > - ir_emit_vertex()
> > > - : ir_instruction(ir_type_emit_vertex)
> > > + ir_emit_vertex(unsigned stream = 0)
> > > + : ir_instruction(ir_type_emit_vertex),
> > > + stream(stream)
> > > {
> > > }
> > >
> > > @@ -2171,10 +2172,12 @@ public:
> > >
> > > virtual ir_emit_vertex *clone(void *mem_ctx, struct hash_table *)
> > > const
> > > {
> > > - return new(mem_ctx) ir_emit_vertex();
> > > + return new(mem_ctx) ir_emit_vertex(stream);
> > > }
> > >
> > > virtual ir_visitor_status accept(ir_hierarchical_visitor *);
> > > +
> > > + unsigned stream;
> > > };
> > >
> > > /**
> > > @@ -2183,8 +2186,9 @@ public:
> > > */
> > > class ir_end_primitive : public ir_instruction {
> > > public:
> > > - ir_end_primitive()
> > > - : ir_instruction(ir_type_end_primitive)
> > > + ir_end_primitive(unsigned stream = 0)
> > > + : ir_instruction(ir_type_end_primitive),
> > > + stream(stream)
> > > {
> > > }
> > >
> > > @@ -2195,10 +2199,12 @@ public:
> > >
> > > virtual ir_end_primitive *clone(void *mem_ctx, struct hash_table *)
> > > const
> > > {
> > > - return new(mem_ctx) ir_end_primitive();
> > > + return new(mem_ctx) ir_end_primitive(stream);
> > > }
> > >
> > > virtual ir_visitor_status accept(ir_hierarchical_visitor *);
> > > +
> > > + unsigned stream;
> > > };
> > >
> > > /*@}*/
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
