Right, this happens because ir_emit_vertex doesn't take a proper
operand, so it can't keep it alive.
What I think you want to do is change the stream in ir_emit_vertex and
ir_end_primitive to be a pointer to ir_rvalue (and apply the various
tweaks required to consider it alive; have rvalue visitors descend
into it; etc) then emit:
(function EmitStreamVertex
(signature void
(parameters
(declare (const_in ) int stream)
)
(
(emit-vertex (var_ref stream))
))
)
which would inline in your case to
(declare () int stream)
(assign (x) (var_ref stream) (constant int (0)) )
(emit-vertex (var_ref stream))
and then after constant propagation,
(emit-vertex (constant int (0)) )
which you can then pick out in your later visitors just as easily as
you can with the integer you're currently storing.
On Fri, Jun 13, 2014 at 11:52 PM, Iago Toral <ito...@igalia.com> wrote:
> 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
