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
