Re: [Mesa-dev] [PATCH 11/18] glsl: Add support for EmitStreamVertex() and EndStreamPrimitive().
Hi Chris,
On Sat, 2014-06-14 at 08:34 +1200, Chris Forbes wrote:
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:
thanks for the tip, I will look into this. I am not sure about what you
mean by applying the various tweaks required to consider the stream
alive (unless you mean the rvalue visitor stuff specifically), but I'll
try to look for clues in other built-in functions.
Iago
(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
Re: [Mesa-dev] [PATCH 11/18] glsl: Add support for EmitStreamVertex() and EndStreamPrimitive().
Have a look at the ir_texture stuff -- it's complex, but it has a
whole bunch of ir_value* for the various texture parameters. You'll be
able to do something simpler, but will probably have to touch most of
the same places that currently treat texturing specially.
On Mon, Jun 16, 2014 at 7:04 PM, Iago Toral ito...@igalia.com wrote:
Hi Chris,
On Sat, 2014-06-14 at 08:34 +1200, Chris Forbes wrote:
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:
thanks for the tip, I will look into this. I am not sure about what you
mean by applying the various tweaks required to consider the stream
alive (unless you mean the rvalue visitor stuff specifically), but I'll
try to look for clues in other built-in functions.
Iago
(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
Re: [Mesa-dev] [PATCH 11/18] glsl: Add support for EmitStreamVertex() and EndStreamPrimitive().
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
Re: [Mesa-dev] [PATCH 11/18] glsl: Add support for EmitStreamVertex() and EndStreamPrimitive().
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
Re: [Mesa-dev] [PATCH 11/18] glsl: Add support for EmitStreamVertex() and EndStreamPrimitive().
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
Re: [Mesa-dev] [PATCH 11/18] glsl: Add support for EmitStreamVertex() and EndStreamPrimitive().
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
[Mesa-dev] [PATCH 11/18] glsl: Add support for EmitStreamVertex() and EndStreamPrimitive().
---
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
Re: [Mesa-dev] [PATCH 11/18] glsl: Add support for EmitStreamVertex() and EndStreamPrimitive().
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,
