On Fri, 2014-06-13 at 10:28 +0200, Iago Toral wrote:
> 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.
The variable is killed because it is not used, as I was anticipating in
my patch, but I don't think the optimization passes are broken, I think
this is expected to happen:
This is the code generated for EmitStreamVertex(0) after function
inlining:
(declare () int stream)
(assign (x) (var_ref stream) (constant int (0)) )
(emit-vertex)
(...)
(function EmitStreamVertex
(signature void
(parameters
(declare (const_in ) int stream)
)
(
(emit-vertex)
))
)
And this is after the dead code elimination passes (dead_code and
dead_code_local), first the assignment is removed:
(declare () int stream)
(emit-vertex)
And finally, in a second pass, it removes the declaration too, leaving:
(emit-vertex)
Seems to make sense: it is killing a variable that is, at this stage,
not used for anything. So, as I was saying in the original patch, I
think we can't do EmitStreamVertex(n) like any other built-in function
because we won't be using its input parameter in the body of the
function for anything, the variable's value is to be used in the visitor
when it is time to generate the native code and that happens after the
optimization passes, so we need to grab its constant value before the
optimization passes (as my original patch did) or we have to find a way
to tell the optimization passes that it should not touch this variable
specifically (and then we would still have to figure out how to access
that temporary variable holding the stream value from the visitor).
Iago
> 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
