Re: [Mesa-dev] [PATCH 1/5] glsl: Move get_error_instruction() from ir_call to ir_constant.
On Tue, 20 Sep 2011 18:28:15 -0700, Kenneth Graunke kenn...@whitecape.org wrote: All this does is generate a bogus value with error type; the fact that it was in ir_call was rather arbitrary to begin with. ir_constant is an equally arbitrary place. The rationale is that a future commit will change ir_calls from rvalues to statements, and all uses of this function expect an rvalue. This would make a lot more sense to me as a global get_error_rvalue(ctx) instead of a method of some arbitrary class. pgpyCitQwVVJD.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/5] glsl: Move get_error_instruction() from ir_call to ir_constant.
On 20 September 2011 18:28, Kenneth Graunke kenn...@whitecape.org wrote:
diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
index d6594cd..70d0ae2 100644
--- a/src/glsl/ir.cpp
+++ b/src/glsl/ir.cpp
@@ -725,6 +725,15 @@ ir_constant::zero(void *mem_ctx, const glsl_type
*type)
return c;
}
+ir_constant *
+ir_constant::error_value(void *ctx)
+{
+ ir_constant *ir = new(ctx) ir_constant;
+
+ ir-type = glsl_type::error_type;
+ return ir;
+}
+
bool
ir_constant::get_bool_component(unsigned i) const
{
@@ -1446,16 +1455,6 @@ ir_function::has_user_signature()
return false;
}
-
-ir_call *
-ir_call::get_error_instruction(void *ctx)
-{
- ir_call *call = new(ctx) ir_call;
-
- call-type = glsl_type::error_type;
- return call;
-}
-
I believe this was the only code that used the private constructor
ir_call::ir_call() (the one that took no parameters), so we should probably
remove that too.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/5] glsl: Move get_error_instruction() from ir_call to ir_constant.
On 09/21/2011 10:21 AM, Eric Anholt wrote: On Tue, 20 Sep 2011 18:28:15 -0700, Kenneth Graunke kenn...@whitecape.org wrote: All this does is generate a bogus value with error type; the fact that it was in ir_call was rather arbitrary to begin with. ir_constant is an equally arbitrary place. The rationale is that a future commit will change ir_calls from rvalues to statements, and all uses of this function expect an rvalue. This would make a lot more sense to me as a global get_error_rvalue(ctx) instead of a method of some arbitrary class. I agree that putting it in ir_constant is quite arbitrary, but I haven't found a better solution. It has to be an instance of _some_ class; I chose ir_constant since it's a leaf node (well, except for structures/arrays, I guess.) Here are some options I've thought about: 1. Make it an instance of ir_rvalue itself. The problem here is that ir_rvalue can't be instantiated, as it has pure virtual methods (clone, constant_expression_value). I could implement those (clone would make a new error_type object, CE would return NULL). But this feels dirty; I'd really rather keep it as an abstract base class. 2. Create a new ir_error subclass of ir_rvalue. This is clean, but aggrandizes something that's used so little. I'm OK with doing this if you'd prefer it. I'm honestly not a fan of error_type. We've had a lot of crashes due to someone inserting half-baked IR (i.e. ir_expression with NULL operands) with type set to error_type. It'd probably be nicer to have a leaf-node error value singleton. Maybe an ir_error class would be a step in the right direction (though we'd also need to make ir_rvalues not exec_nodes too). Still, I think ir_constant is at least better than ir_call. Should we go with that for now, or, what would you prefer? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/5] glsl: Move get_error_instruction() from ir_call to ir_constant.
On 21 September 2011 10:21, Eric Anholt e...@anholt.net wrote: On Tue, 20 Sep 2011 18:28:15 -0700, Kenneth Graunke kenn...@whitecape.org wrote: All this does is generate a bogus value with error type; the fact that it was in ir_call was rather arbitrary to begin with. ir_constant is an equally arbitrary place. The rationale is that a future commit will change ir_calls from rvalues to statements, and all uses of this function expect an rvalue. This would make a lot more sense to me as a global get_error_rvalue(ctx) instead of a method of some arbitrary class. I guess there are two questions to be answered here: 1. What type of object should we use as a placeholder in the IR for errors? 2. Where should we put the factory method that creates this object? As to question 1, we have to choose some type, and short of creating a brand new type to represent errors (which seems wasteful and possibly bug-prone), ir_constant is as good as any. It's certainly better than ir_call for the reasons Ken stated. As to question 2, I agree that making this a static method of ir_constant seems kind of arbitrary--ideally the caller shouldn't know or care that the factory method creates an ir_constant. But I'd prefer to make it a static method of some class so that it's less likely to be stumbled upon by someone who doesn't need it. How about making it a static method of ir_rvalue? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/5] glsl: Move get_error_instruction() from ir_call to ir_constant.
On 09/21/2011 11:48 AM, Paul Berry wrote:
On 20 September 2011 18:28, Kenneth Graunke wrote:
@@ -1446,16 +1455,6 @@ ir_function::has_user_signature()
return false;
}
-
-ir_call *
-ir_call::get_error_instruction(void *ctx)
-{
- ir_call *call = new(ctx) ir_call;
-
- call-type = glsl_type::error_type;
- return call;
-}
-
I believe this was the only code that used the private constructor
ir_call::ir_call() (the one that took no parameters), so we should
probably remove that too.
Good call. Updated to remove that.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/5] glsl: Move get_error_instruction() from ir_call to ir_constant.
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 09/20/2011 06:28 PM, Kenneth Graunke wrote:
| All this does is generate a bogus value with error type; the fact
| that it was in ir_call was rather arbitrary to begin with.
| ir_constant is an equally arbitrary place. The rationale is that a
| future commit will change ir_calls from rvalues to statements, and
| all uses of this function expect an rvalue.
There was nothing arbitrary about it. It generates an invalid ir_call
when encountering an error while processing an
ast_function_expression. At the time the thinking was that there
would be other get_error_instruction methods in other class. The
thinking was also that objects in expression trees might eventually
become flyweights. Neither of those things ever happened.
| Signed-off-by: Kenneth Graunkekenn...@whitecape.org ---
| src/glsl/ast_function.cpp| 32
| src/glsl/hir_field_selection.cpp |
| 2 +- src/glsl/ir.cpp | 19 +--
| src/glsl/ir.h| 14 +++---
| src/glsl/ir_clone.cpp|5 ++--- 5 files changed, 35
| insertions(+), 37 deletions(-)
|
| diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
| index ca45934..392b7f9 100644 --- a/src/glsl/ast_function.cpp +++
| b/src/glsl/ast_function.cpp @@ -164,7 +164,7 @@
| match_function_by_name(exec_list *instructions, const char *name,
| _mesa_glsl_error(loc, state, parameter `%s' must be a constant
| expression, formal-name); - return
| ir_call::get_error_instruction(ctx); +return
| ir_constant::error_value(ctx); }
|
| if ((formal-mode == ir_var_out) @@ -328,7 +328,7 @@
| match_function_by_name(exec_list *instructions, const char *name,
|
| }
|
| - return ir_call::get_error_instruction(ctx); + return
| ir_constant::error_value(ctx); } }
|
| @@ -504,7 +504,7 @@ process_array_constructor(exec_list
| *instructions, parameter%s, (constructor_type-length != 0) ? at
| least : exactly, min_param, (min_param= 1) ? : s); -
| return ir_call::get_error_instruction(ctx); + return
| ir_constant::error_value(ctx); }
|
| if (constructor_type-length == 0) { @@ -1141,7 +1141,7 @@
| ast_function_expression::hir(exec_list *instructions,
| _mesa_glsl_error( loc, state, unknown type `%s' (structure name
| may be shadowed by a variable with the same name),
| type-type_name); - return ir_call::get_error_instruction(ctx); +
| return ir_constant::error_value(ctx); }
|
|
| @@ -1150,14 +1150,14 @@ ast_function_expression::hir(exec_list
| *instructions, if (constructor_type-is_sampler()) {
| _mesa_glsl_error( loc, state, cannot construct sampler type
| `%s', constructor_type-name); - return
| ir_call::get_error_instruction(ctx); + return
| ir_constant::error_value(ctx); }
|
| if (constructor_type-is_array()) { if (state-language_version=
| 110) { _mesa_glsl_error( loc, state, array constructors
| forbidden in GLSL 1.10); - return
| ir_call::get_error_instruction(ctx); +return
| ir_constant::error_value(ctx); }
|
| return process_array_constructor(instructions, constructor_type, @@
| -1188,7 +1188,7 @@ ast_function_expression::hir(exec_list
| *instructions, insufficient parameters to constructor for
| `%s', constructor_type-name); -return
| ir_call::get_error_instruction(ctx); + return
| ir_constant::error_value(ctx); }
|
| if
| (apply_implicit_conversion(constructor_type-fields.structure[i].type,
|
|
@@ -1202,7 +1202,7 @@ ast_function_expression::hir(exec_list *instructions,
| constructor_type-fields.structure[i].name, ir-type-name,
| constructor_type-fields.structure[i].type-name); - return
| ir_call::get_error_instruction(ctx);; + return
| ir_constant::error_value(ctx);; }
|
| node = node-next; @@ -1211,7 +1211,7 @@
| ast_function_expression::hir(exec_list *instructions, if
| (!node-is_tail_sentinel()) { _mesa_glsl_error(loc, state, too
| many parameters in constructor for `%s',
| constructor_type-name); - return
| ir_call::get_error_instruction(ctx); +return
| ir_constant::error_value(ctx); }
|
| ir_rvalue *const constant = @@ -1225,7 +1225,7 @@
| ast_function_expression::hir(exec_list *instructions, }
|
| if (!constructor_type-is_numeric()
| !constructor_type-is_boolean()) - return
| ir_call::get_error_instruction(ctx); + return
| ir_constant::error_value(ctx);
|
| /* Total number of components of the type being constructed. */
| const unsigned type_components = constructor_type-components(); @@
| -1252,14 +1252,14 @@ ast_function_expression::hir(exec_list
| *instructions, _mesa_glsl_error( loc, state, too many parameters
| to `%s' constructor, constructor_type-name); - return
| ir_call::get_error_instruction(ctx); +return
| ir_constant::error_value(ctx); }
|
| if (!result-type-is_numeric() !result-type-is_boolean()) {
| _mesa_glsl_error( loc, state, cannot construct `%s' from
