Re: [Mesa-dev] [PATCH 2/4] glsl: Emit function signatures at toplevel, even for built-ins.

2011-08-05 Thread Paul Berry 
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.

2011-08-02 Thread Paul Berry 
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.

2011-08-02 Thread Ian Romanick 
-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.

2011-08-01 Thread Paul Berry 
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.

2011-08-01 Thread Ian Romanick 
-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
 ---