Re: [Mesa-dev] [PATCH 2/4] glsl: Emit function signatures at toplevel, even for built-ins.
On 2 August 2011 18:02, Ian Romanick i...@freedesktop.org wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 08/02/2011 12:18 PM, Paul Berry wrote: The case this patch is designed to fix is when a built-in function is called from inside the constant integer expression that specifies the length of an array, e.g.: #version 120 float x[cos(3.14159/2.0) 0.5 ? 1 : 2]; This case fails before the patch, because when processing an array length, we are emitting IR into a dummy exec_list (see process_array_type() in ast_to_hir.cpp). process_array_type() later checks (via an assertion) that no instructions were emitted to the dummy exec_list, based on the reasonable assumption that we shouldn't need to emit instructions to calculate the value of a constant. This This is the bit of text that should be in the commit message. :) Now it makes sense. The bit about the dummy exec_list is the important part... that people familiar with the code may have forgotten. Ok, I've updated the commit message to read as follows: glsl: Emit function signatures at toplevel, even for built-ins. The ast-to-hir conversion needs to emit function signatures in two circumstances: when a function declaration (or definition) is encountered, and when a built-in function is encountered. To avoid emitting a function signature in an illegal place (such as inside a function), emit_function() checked whether we were inside a function definition, and if so, emitted the signature before the function definition. However, this didn't cover the case of emitting function signatures for built-in functions when those built-in functions are called from inside the constant integer expression that specifies the length of a global array. This failed because when processing an array length, we are emitting IR into a dummy exec_list (see process_array_type() in ast_to_hir.cpp). process_array_type() later checks (via an assertion) that no instructions were emitted to the dummy exec_list, based on the reasonable assumption that we shouldn't need to emit instructions to calculate the value of a constant. This patch changes emit_function() so that it emits function signatures at toplevel in all cases. This partially fixes bug 38625 (https://bugs.freedesktop.org/show_bug.cgi?id=38625). The remainder of the fix is in the patch that follows. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl: Emit function signatures at toplevel, even for built-ins.
On 1 August 2011 18:08, Ian Romanick i...@freedesktop.org wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 08/01/2011 04:07 PM, Paul Berry wrote: The ast-to-hir conversion needs to emit function signatures in two circumstances: when a function declaration (or definition) is encountered, and when a built-in function is encountered. To avoid emitting a function signature in an illegal place (such as inside a function), emit_function() checked whether we were inside a function definition, and if so, emitted the signature before the function definition. However, this didn't cover the case of emitting function signatures for built-in functions when those built-in functions are called from global scope (e.g. a built-in function called from inside the constant integer expression that specifies the length of an array). This patch changes emit_function() so that it emits function signatures at toplevel in all cases. There's something about this patch that I don't grok. The state-current_function test in emit function exists specifically to handle calls to functions (built-in or otherwise) at global scope. Without that, code such as the snippet below would not work, and text in the commit log seems to indicate that it shouldn't work. However, I verified that it does. #version 120 const float x = cos(3.14159 / 2.0); I'm confused. Do you mean to say that you didn't expect the above code to work before applying this patch, or you didn't expect the above code to work after applying this patch? Before applying this patch, the above code works because when processing a global initializer, we are emitting IR at toplevel scope. Since state-current_function is NULL, emit_function() just emits the declaration in place, which is fine because function declarations are allowed at toplevel scope. After applying this patch, the above code still works because emit_function() always emits the declaration at toplevel scope. The case this patch is designed to fix is when a built-in function is called from inside the constant integer expression that specifies the length of an array, e.g.: #version 120 float x[cos(3.14159/2.0) 0.5 ? 1 : 2]; This case fails before the patch, because when processing an array length, we are emitting IR into a dummy exec_list (see process_array_type() in ast_to_hir.cpp). process_array_type() later checks (via an assertion) that no instructions were emitted to the dummy exec_list, based on the reasonable assumption that we shouldn't need to emit instructions to calculate the value of a constant. This is the crux of bug 38625, which is the bug I'm trying to fix in this patch series. After applying this patch, the situation is improved, but the bug still isn't fixed. In addition to this change, we need to avoid emitting a function call when constant-folding a builtin function. That's the subject of the next patch in the series. I should have made this clearer in the commit message. I'll update it to clarify. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl: Emit function signatures at toplevel, even for built-ins.
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 08/02/2011 12:18 PM, Paul Berry wrote: On 1 August 2011 18:08, Ian Romanick i...@freedesktop.org wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 08/01/2011 04:07 PM, Paul Berry wrote: The ast-to-hir conversion needs to emit function signatures in two circumstances: when a function declaration (or definition) is encountered, and when a built-in function is encountered. To avoid emitting a function signature in an illegal place (such as inside a function), emit_function() checked whether we were inside a function definition, and if so, emitted the signature before the function definition. However, this didn't cover the case of emitting function signatures for built-in functions when those built-in functions are called from global scope (e.g. a built-in function called from inside the constant integer expression that specifies the length of an array). This patch changes emit_function() so that it emits function signatures at toplevel in all cases. There's something about this patch that I don't grok. The state-current_function test in emit function exists specifically to handle calls to functions (built-in or otherwise) at global scope. Without that, code such as the snippet below would not work, and text in the commit log seems to indicate that it shouldn't work. However, I verified that it does. #version 120 const float x = cos(3.14159 / 2.0); I'm confused. Do you mean to say that you didn't expect the above code to work before applying this patch, or you didn't expect the above code to work after applying this patch? I'm saying that it does work, and it is supposed to work. Your commit message implies that it does not work before the patch. Before applying this patch, the above code works because when processing a global initializer, we are emitting IR at toplevel scope. Since state-current_function is NULL, emit_function() just emits the declaration in place, which is fine because function declarations are allowed at toplevel scope. After applying this patch, the above code still works because emit_function() always emits the declaration at toplevel scope. The case this patch is designed to fix is when a built-in function is called from inside the constant integer expression that specifies the length of an array, e.g.: #version 120 float x[cos(3.14159/2.0) 0.5 ? 1 : 2]; This case fails before the patch, because when processing an array length, we are emitting IR into a dummy exec_list (see process_array_type() in ast_to_hir.cpp). process_array_type() later checks (via an assertion) that no instructions were emitted to the dummy exec_list, based on the reasonable assumption that we shouldn't need to emit instructions to calculate the value of a constant. This This is the bit of text that should be in the commit message. :) Now it makes sense. The bit about the dummy exec_list is the important part... that people familiar with the code may have forgotten. is the crux of bug 38625, which is the bug I'm trying to fix in this patch series. After applying this patch, the situation is improved, but the bug still isn't fixed. In addition to this change, we need to avoid emitting a function call when constant-folding a builtin function. That's the subject of the next patch in the series. I should have made this clearer in the commit message. I'll update it to clarify. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAk44nhsACgkQX1gOwKyEAw/l6ACfaBeNcJLpoWi1ZDthKrL46OHa Re8AnRGplb1Pz0D2dN26CKTcMYz+3OR6 =c4uK -END PGP SIGNATURE- ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/4] glsl: Emit function signatures at toplevel, even for built-ins.
The ast-to-hir conversion needs to emit function signatures in two circumstances: when a function declaration (or definition) is encountered, and when a built-in function is encountered. To avoid emitting a function signature in an illegal place (such as inside a function), emit_function() checked whether we were inside a function definition, and if so, emitted the signature before the function definition. However, this didn't cover the case of emitting function signatures for built-in functions when those built-in functions are called from global scope (e.g. a built-in function called from inside the constant integer expression that specifies the length of an array). This patch changes emit_function() so that it emits function signatures at toplevel in all cases. --- src/glsl/ast.h|3 +-- src/glsl/ast_function.cpp |2 +- src/glsl/ast_to_hir.cpp | 31 ++- src/glsl/glsl_parser_extras.h |6 ++ 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/glsl/ast.h b/src/glsl/ast.h index 878f48b..d1de227 100644 --- a/src/glsl/ast.h +++ b/src/glsl/ast.h @@ -730,7 +730,6 @@ _mesa_ast_field_selection_to_hir(const ast_expression *expr, struct _mesa_glsl_parse_state *state); void -emit_function(_mesa_glsl_parse_state *state, exec_list *instructions, - ir_function *f); +emit_function(_mesa_glsl_parse_state *state, ir_function *f); #endif /* AST_H */ diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp index 8bcf48d..34a82f8 100644 --- a/src/glsl/ast_function.cpp +++ b/src/glsl/ast_function.cpp @@ -125,7 +125,7 @@ match_function_by_name(exec_list *instructions, const char *name, if (f == NULL) { f = new(ctx) ir_function(name); state-symbols-add_global_function(f); - emit_function(state, instructions, f); + emit_function(state, f); } f-add_signature(sig-clone_prototype(f, NULL)); diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp index c0524bf..c1f160e 100644 --- a/src/glsl/ast_to_hir.cpp +++ b/src/glsl/ast_to_hir.cpp @@ -66,6 +66,8 @@ _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state) state-current_function = NULL; + state-toplevel_ir = instructions; + /* Section 4.2 of the GLSL 1.20 specification states: * The built-in functions are scoped in a scope outside the global scope * users declare global variables in. That is, a shader's global scope, @@ -85,6 +87,8 @@ _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state) ast-hir(instructions, state); detect_recursion_unlinked(state, instructions); + + state-toplevel_ir = NULL; } @@ -2926,23 +2930,16 @@ ast_parameter_declarator::parameters_to_hir(exec_list *ast_parameters, void -emit_function(_mesa_glsl_parse_state *state, exec_list *instructions, - ir_function *f) +emit_function(_mesa_glsl_parse_state *state, ir_function *f) { - /* Emit the new function header */ - if (state-current_function == NULL) { - instructions-push_tail(f); - } else { - /* IR invariants disallow function declarations or definitions nested - * within other function definitions. Insert the new ir_function - * block in the instruction sequence before the ir_function block - * containing the current ir_function_signature. - */ - ir_function *const curr = -const_castir_function *(state-current_function-function()); - - curr-insert_before(f); - } + /* IR invariants disallow function declarations or definitions +* nested within other function definitions. But there is no +* requirement about the relative order of function declarations +* and definitions with respect to one another. So simply insert +* the new ir_function block at the end of the toplevel instruction +* list. +*/ + state-toplevel_ir-push_tail(f); } @@ -3069,7 +3066,7 @@ ast_function::hir(exec_list *instructions, return NULL; } - emit_function(state, instructions, f); + emit_function(state, f); } /* Verify the return type of main() */ diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h index 2f4d3cb..fc392da 100644 --- a/src/glsl/glsl_parser_extras.h +++ b/src/glsl/glsl_parser_extras.h @@ -129,6 +129,12 @@ struct _mesa_glsl_parse_state { */ class ir_function_signature *current_function; + /** +* During AST to IR conversion, pointer to the toplevel IR +* instruction list being generated. +*/ + exec_list *toplevel_ir; + /** Have we found a return statement in this function? */ bool found_return; -- 1.7.6 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] glsl: Emit function signatures at toplevel, even for built-ins.
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 08/01/2011 04:07 PM, Paul Berry wrote: The ast-to-hir conversion needs to emit function signatures in two circumstances: when a function declaration (or definition) is encountered, and when a built-in function is encountered. To avoid emitting a function signature in an illegal place (such as inside a function), emit_function() checked whether we were inside a function definition, and if so, emitted the signature before the function definition. However, this didn't cover the case of emitting function signatures for built-in functions when those built-in functions are called from global scope (e.g. a built-in function called from inside the constant integer expression that specifies the length of an array). This patch changes emit_function() so that it emits function signatures at toplevel in all cases. There's something about this patch that I don't grok. The state-current_function test in emit function exists specifically to handle calls to functions (built-in or otherwise) at global scope. Without that, code such as the snippet below would not work, and text in the commit log seems to indicate that it shouldn't work. However, I verified that it does. #version 120 const float x = cos(3.14159 / 2.0); --- src/glsl/ast.h|3 +-- src/glsl/ast_function.cpp |2 +- src/glsl/ast_to_hir.cpp | 31 ++- src/glsl/glsl_parser_extras.h |6 ++ 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/glsl/ast.h b/src/glsl/ast.h index 878f48b..d1de227 100644 --- a/src/glsl/ast.h +++ b/src/glsl/ast.h @@ -730,7 +730,6 @@ _mesa_ast_field_selection_to_hir(const ast_expression *expr, struct _mesa_glsl_parse_state *state); void -emit_function(_mesa_glsl_parse_state *state, exec_list *instructions, - ir_function *f); +emit_function(_mesa_glsl_parse_state *state, ir_function *f); #endif /* AST_H */ diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp index 8bcf48d..34a82f8 100644 --- a/src/glsl/ast_function.cpp +++ b/src/glsl/ast_function.cpp @@ -125,7 +125,7 @@ match_function_by_name(exec_list *instructions, const char *name, if (f == NULL) { f = new(ctx) ir_function(name); state-symbols-add_global_function(f); -emit_function(state, instructions, f); +emit_function(state, f); } f-add_signature(sig-clone_prototype(f, NULL)); diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp index c0524bf..c1f160e 100644 --- a/src/glsl/ast_to_hir.cpp +++ b/src/glsl/ast_to_hir.cpp @@ -66,6 +66,8 @@ _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state) state-current_function = NULL; + state-toplevel_ir = instructions; + /* Section 4.2 of the GLSL 1.20 specification states: * The built-in functions are scoped in a scope outside the global scope * users declare global variables in. That is, a shader's global scope, @@ -85,6 +87,8 @@ _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state) ast-hir(instructions, state); detect_recursion_unlinked(state, instructions); + + state-toplevel_ir = NULL; } @@ -2926,23 +2930,16 @@ ast_parameter_declarator::parameters_to_hir(exec_list *ast_parameters, void -emit_function(_mesa_glsl_parse_state *state, exec_list *instructions, - ir_function *f) +emit_function(_mesa_glsl_parse_state *state, ir_function *f) { - /* Emit the new function header */ - if (state-current_function == NULL) { - instructions-push_tail(f); - } else { - /* IR invariants disallow function declarations or definitions nested - * within other function definitions. Insert the new ir_function - * block in the instruction sequence before the ir_function block - * containing the current ir_function_signature. - */ - ir_function *const curr = - const_castir_function *(state-current_function-function()); - - curr-insert_before(f); - } + /* IR invariants disallow function declarations or definitions +* nested within other function definitions. But there is no +* requirement about the relative order of function declarations +* and definitions with respect to one another. So simply insert +* the new ir_function block at the end of the toplevel instruction +* list. +*/ + state-toplevel_ir-push_tail(f); } @@ -3069,7 +3066,7 @@ ast_function::hir(exec_list *instructions, return NULL; } - emit_function(state, instructions, f); + emit_function(state, f); } /* Verify the return type of main() */ diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h index 2f4d3cb..fc392da 100644 ---