Re: [Mesa-dev] [PATCH 3/3] glsl: Perform implicit type conversions on function call out parameters.

2011-08-15 Thread Paul Berry
Have you had a chance to think about any of these issues, Ian?  I'd
like to commit this patch series to Mesa.

On 5 August 2011 17:16, Paul Berry stereotype...@gmail.com wrote:
 On 2 August 2011 18:27, Ian Romanick i...@freedesktop.org wrote:
 +       *
 +       * Also, perform implicit conversion of arguments.  Note: to
 +       * implicitly convert out parameters, we need to place them in a
 +       * temporary variable, and do the conversion after the call
 +       * takes place.  Since we haven't emitted the call yet, we'll
 +       * place the post-call conversions in a temporary exec_list, and
 +       * emit them later.

 It looks like this comment (and the others below) are wrapped to much
 less than 80 columns.

 They are wrapped to 70 columns, which is the default value used by
 Emacs and appeals to me personally.  But I care way less about
 formatting details like this than I care about not losing time to
 debating them, and I think the best way to avoid that is to pick a
 standard and document it.  What's your preferred line-wrapping width,
 and would you be willing to enshrine it in docs/devinfo.html (where it
 might, I fear, be subject to a round of bike-shedding by those with
 strong feelings about such things)?  If so, I'd be glad to update my
 Emacs configuration to follow it.


         */
        exec_list_iterator actual_iter = actual_parameters-iterator();
        exec_list_iterator formal_iter = sig-parameters.iterator();
 @@ -185,8 +194,50 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
        }

        if (formal-type-is_numeric() || formal-type-is_boolean()) {
 -         ir_rvalue *converted = convert_component(actual, formal-type);
 -         actual-replace_with(converted);
 +            switch (formal-mode) {
 +            case ir_var_in:
 +               {

 The { should be on the same line as the case, and emacs got the
 indentation wonkey because it's not.

 For the record, this was actually completely on purpose.  I prefer for
 an opening brace after a case label to be on its own line, and to
 cause an extra level of indent, to emphasize the fact that the brace
 exists solely for the purpose of variable scoping, and is not part of
 the syntax of the switch statement (this is in contrast with the {
 that comes after an if statement, for example, which is part of the
 syntax of if and not solely for scoping).  Incidentally, the
 behavior of indent -br -i3 -npcs --no-tabs (which is recommended by
 devinfo.html) seems to agree with my formatting--it doesn't alter this
 case block as I submitted it, though it does force the opening braces
 of if-statements to be on the same line as the if.  And if I force
 the { to the same line as the case, the indent command continues to
 generate the indentation that appears wonky to you.

 I'm guessing from looking at code elsewhere in Mesa that what you
 would have preferred would have been this, correct?

 switch (formal-mode) {
 case ir_var_in: {
   ir_rvalue *converted
      = convert_component(actual, formal-type);
   actual-replace_with(converted);
   break;
 }
 case ir_var_out:
   ...
 }

 Normally I wouldn't bike-shed something like this, but what bothers me
 about the above is that it puts the closing brace of the case at the
 same indentation as the closing brace of the switch statement.  That
 makes it hard to tell at a glance which closing brace terminates the
 switch statement and which closing brace terminates the case, and that
 is IMHO the whole point of indentation.

 However, having said all that, I realize that I'm still fairly new to
 the mesa project and I may just have to suck it up and conform to the
 mesa rules.  I just wanted to give you a chance to change your mind :)


 +                  ir_rvalue *converted
 +                     = convert_component(actual, formal-type);
 +                  actual-replace_with(converted);
 +               }
 +               break;
 +            case ir_var_out:
 +               if (actual-type != formal-type)
 +               {

 The { should be on the same line as the if.

 _This_ was a complete oversight on my part.  I'll fix it.


 +                  /* Create a temporary variable to hold the out
 +                   * parameter.  Don't pass a hashtable to clone()
 +                   * because we don't want this temporary variable to
 +                   * be in scope.
 +                   */
 +                  ir_variable *tmp = formal-clone(ctx, NULL);
 +                  tmp-mode = ir_var_auto;

 ir_var_auto is only used for user-defined variables, and
 ir_var_temporary is used for compiler-generated variables.  We'd also
 usually do something like:

    ir_variable *tmp =
        new(mem_ctx) ir_variable(formal-type,
                                 out_parameter_conversion,
                                 ir_var_temporary);

 Giving it a name like that makes the origin of the variable clear in
 shader dumps.  This has proven to be an invaluable 

Re: [Mesa-dev] [PATCH 3/3] glsl: Perform implicit type conversions on function call out parameters.

2011-08-15 Thread Ian Romanick
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 08/05/2011 05:16 PM, Paul Berry wrote:
 On 2 August 2011 18:27, Ian Romanick i...@freedesktop.org wrote:
 +   *
 +   * Also, perform implicit conversion of arguments.  Note: to
 +   * implicitly convert out parameters, we need to place them in a
 +   * temporary variable, and do the conversion after the call
 +   * takes place.  Since we haven't emitted the call yet, we'll
 +   * place the post-call conversions in a temporary exec_list, and
 +   * emit them later.

 It looks like this comment (and the others below) are wrapped to much
 less than 80 columns.
 
 They are wrapped to 70 columns, which is the default value used by
 Emacs and appeals to me personally.  But I care way less about
 formatting details like this than I care about not losing time to
 debating them, and I think the best way to avoid that is to pick a
 standard and document it.  What's your preferred line-wrapping width,
 and would you be willing to enshrine it in docs/devinfo.html (where it
 might, I fear, be subject to a round of bike-shedding by those with
 strong feelings about such things)?  If so, I'd be glad to update my
 Emacs configuration to follow it.

It's more a matter of consistency within a file.  When all the other
comment blocks are formatted to 77-79 columns, the one block that is
formatted to 70 columns really stands out.  This is even more the case
when the formatting change happens within a single comment block.

Looking at Mesa code, I believe that everyone (tries) to wrap comments
and code to somewhere between 77 and 79 columns.  Somethings go over
(like the extension table in extensions.c), and something go under.  I
usually have my fill-column set to 78.

 */
exec_list_iterator actual_iter = actual_parameters-iterator();
exec_list_iterator formal_iter = sig-parameters.iterator();
 @@ -185,8 +194,50 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
}

if (formal-type-is_numeric() || formal-type-is_boolean()) {
 - ir_rvalue *converted = convert_component(actual, formal-type);
 - actual-replace_with(converted);
 +switch (formal-mode) {
 +case ir_var_in:
 +   {

 The { should be on the same line as the case, and emacs got the
 indentation wonkey because it's not.
 
 For the record, this was actually completely on purpose.  I prefer for
 an opening brace after a case label to be on its own line, and to
 cause an extra level of indent, to emphasize the fact that the brace
 exists solely for the purpose of variable scoping, and is not part of
 the syntax of the switch statement (this is in contrast with the {
 that comes after an if statement, for example, which is part of the
 syntax of if and not solely for scoping).  Incidentally, the
 behavior of indent -br -i3 -npcs --no-tabs (which is recommended by
 devinfo.html) seems to agree with my formatting--it doesn't alter this
 case block as I submitted it, though it does force the opening braces
 of if-statements to be on the same line as the if.  And if I force
 the { to the same line as the case, the indent command continues to
 generate the indentation that appears wonky to you.
 
 I'm guessing from looking at code elsewhere in Mesa that what you
 would have preferred would have been this, correct?

Correct is a funny word on matters of style.  Matches the majority of
they other switch-statements in Mesa would be more accurate.

 switch (formal-mode) {
 case ir_var_in: {
ir_rvalue *converted
   = convert_component(actual, formal-type);
actual-replace_with(converted);
break;
 }
 case ir_var_out:
...
 }
 
 Normally I wouldn't bike-shed something like this, but what bothers me
 about the above is that it puts the closing brace of the case at the
 same indentation as the closing brace of the switch statement.  That
 makes it hard to tell at a glance which closing brace terminates the
 switch statement and which closing brace terminates the case, and that
 is IMHO the whole point of indentation.

I'm not a big fan of that either, and I never have been.  The mechanism
that I used before coming to open source projects was to indent the case
line.  I believe the rationale behind this formatting is so that cases
with blocks and without are at the same indentation.  There is some
merit in that.  This also means that cases that start without a block
won't have spurious whitespace changes if a block is added later.

switch () {
case 0:
foo();
break;

case 1: {
...
break;
}

default:
...
break;
}

 However, having said all that, I realize that I'm still fairly new to
 the mesa project and I may just have to suck it up and conform to the
 mesa rules.  I just wanted to give you a chance to change your mind :)
-BEGIN PGP 

Re: [Mesa-dev] [PATCH 3/3] glsl: Perform implicit type conversions on function call out parameters.

2011-08-05 Thread Paul Berry
On 2 August 2011 18:27, Ian Romanick i...@freedesktop.org wrote:
 +       *
 +       * Also, perform implicit conversion of arguments.  Note: to
 +       * implicitly convert out parameters, we need to place them in a
 +       * temporary variable, and do the conversion after the call
 +       * takes place.  Since we haven't emitted the call yet, we'll
 +       * place the post-call conversions in a temporary exec_list, and
 +       * emit them later.

 It looks like this comment (and the others below) are wrapped to much
 less than 80 columns.

They are wrapped to 70 columns, which is the default value used by
Emacs and appeals to me personally.  But I care way less about
formatting details like this than I care about not losing time to
debating them, and I think the best way to avoid that is to pick a
standard and document it.  What's your preferred line-wrapping width,
and would you be willing to enshrine it in docs/devinfo.html (where it
might, I fear, be subject to a round of bike-shedding by those with
strong feelings about such things)?  If so, I'd be glad to update my
Emacs configuration to follow it.


         */
        exec_list_iterator actual_iter = actual_parameters-iterator();
        exec_list_iterator formal_iter = sig-parameters.iterator();
 @@ -185,8 +194,50 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
        }

        if (formal-type-is_numeric() || formal-type-is_boolean()) {
 -         ir_rvalue *converted = convert_component(actual, formal-type);
 -         actual-replace_with(converted);
 +            switch (formal-mode) {
 +            case ir_var_in:
 +               {

 The { should be on the same line as the case, and emacs got the
 indentation wonkey because it's not.

For the record, this was actually completely on purpose.  I prefer for
an opening brace after a case label to be on its own line, and to
cause an extra level of indent, to emphasize the fact that the brace
exists solely for the purpose of variable scoping, and is not part of
the syntax of the switch statement (this is in contrast with the {
that comes after an if statement, for example, which is part of the
syntax of if and not solely for scoping).  Incidentally, the
behavior of indent -br -i3 -npcs --no-tabs (which is recommended by
devinfo.html) seems to agree with my formatting--it doesn't alter this
case block as I submitted it, though it does force the opening braces
of if-statements to be on the same line as the if.  And if I force
the { to the same line as the case, the indent command continues to
generate the indentation that appears wonky to you.

I'm guessing from looking at code elsewhere in Mesa that what you
would have preferred would have been this, correct?

switch (formal-mode) {
case ir_var_in: {
   ir_rvalue *converted
  = convert_component(actual, formal-type);
   actual-replace_with(converted);
   break;
}
case ir_var_out:
   ...
}

Normally I wouldn't bike-shed something like this, but what bothers me
about the above is that it puts the closing brace of the case at the
same indentation as the closing brace of the switch statement.  That
makes it hard to tell at a glance which closing brace terminates the
switch statement and which closing brace terminates the case, and that
is IMHO the whole point of indentation.

However, having said all that, I realize that I'm still fairly new to
the mesa project and I may just have to suck it up and conform to the
mesa rules.  I just wanted to give you a chance to change your mind :)


 +                  ir_rvalue *converted
 +                     = convert_component(actual, formal-type);
 +                  actual-replace_with(converted);
 +               }
 +               break;
 +            case ir_var_out:
 +               if (actual-type != formal-type)
 +               {

 The { should be on the same line as the if.

_This_ was a complete oversight on my part.  I'll fix it.


 +                  /* Create a temporary variable to hold the out
 +                   * parameter.  Don't pass a hashtable to clone()
 +                   * because we don't want this temporary variable to
 +                   * be in scope.
 +                   */
 +                  ir_variable *tmp = formal-clone(ctx, NULL);
 +                  tmp-mode = ir_var_auto;

 ir_var_auto is only used for user-defined variables, and
 ir_var_temporary is used for compiler-generated variables.  We'd also
 usually do something like:

    ir_variable *tmp =
        new(mem_ctx) ir_variable(formal-type,
                                 out_parameter_conversion,
                                 ir_var_temporary);

 Giving it a name like that makes the origin of the variable clear in
 shader dumps.  This has proven to be an invaluable debugging tool.

That seems very reasonable.  I'll make that change.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

Re: [Mesa-dev] [PATCH 3/3] glsl: Perform implicit type conversions on function call out parameters.

2011-08-04 Thread Chad Versace
On 08/02/2011 05:38 PM, Paul Berry wrote:
 When an out parameter undergoes an implicit type conversion, we need
 to store it in a temporary, and then after the call completes, convert
 the resulting value.  In other words, we convert code like the
 following:
 
 void f(out int x);
 float value;
 f(value);
 
 Into IR that's equivalent to this:
 
 void f(out int x);
 float value;
 int x_converted;
 f(x_converted);
 value = float(x_converted);
 
 This transformation needs to happen during ast-to-IR convertion (as
 opposed to, say, a lowering pass), because it is invalid IR for formal
 and actual parameters to have types that don't match.

It is odd that we generate the post_call_conversions IR during ast-to-hir. It is
even more odd that we generate it in a function named match_function_by_name();
after all, it now does much more than a match.

After exploring the code, though, I discovered that match_function_by_name() is
really a misnomer. It is already doing conversion for 'in' parameters, so it
also seems the proper place to do conversion of 'out' parameters.

 
 Fixes piglit tests spec/glsl-1.20/compiler/qualifiers/out-03.vert and
 spec/glsl-1.20/execution/qualifiers/vs-out-{01,02,03}.shader_test, and
 bug 39651.

The Piglit names will need updating after you rename them. Please rename and
commit the Piglit tests before committing this patch.

 
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=39651
 ---
  src/glsl/ast_function.cpp |   64 +---
  1 files changed, 59 insertions(+), 5 deletions(-)
 
 diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
 index 8bcf48d..f945a94 100644
 --- a/src/glsl/ast_function.cpp
 +++ b/src/glsl/ast_function.cpp
 @@ -134,6 +134,8 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
}
 }
  
 +   exec_list post_call_conversions;
 +
 if (sig != NULL) {
/* Verify that 'out' and 'inout' actual parameters are lvalues.  This
 * isn't done in ir_function::matching_signature because that function
 @@ -141,6 +143,13 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
 *
 * Also, validate that 'const_in' formal parameters (an extension of 
 our
 * IR) correspond to ir_constant actual parameters.
 +   *
 +   * Also, perform implicit conversion of arguments.  Note: to
 +   * implicitly convert out parameters, we need to place them in a
 +   * temporary variable, and do the conversion after the call
 +   * takes place.  Since we haven't emitted the call yet, we'll
 +   * place the post-call conversions in a temporary exec_list, and
 +   * emit them later.
 */
exec_list_iterator actual_iter = actual_parameters-iterator();
exec_list_iterator formal_iter = sig-parameters.iterator();
 @@ -185,8 +194,50 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
}
  
if (formal-type-is_numeric() || formal-type-is_boolean()) {
 - ir_rvalue *converted = convert_component(actual, formal-type);
 - actual-replace_with(converted);
 +switch (formal-mode) {
 +case ir_var_in:
 +   {
 +  ir_rvalue *converted
 + = convert_component(actual, formal-type);
 +  actual-replace_with(converted);
 +   }
 +   break;
 +case ir_var_out:
 +   if (actual-type != formal-type)
 +   {
 +  /* Create a temporary variable to hold the out
 +   * parameter.  Don't pass a hashtable to clone()
 +   * because we don't want this temporary variable to
 +   * be in scope.

This comment would be more instructive if it contained this snippet from the
commit message:

As as example, this code
void f(out int x);
float value;
f(value);
is translated into IR equivalent to
void f(out int x);
float value;
int x_converted;
f(x_converted);
value = float(x_converted);

This example goes a long way to helping someone understand how you are juggling
the IR.

 +   */
 +  ir_variable *tmp = formal-clone(ctx, NULL);
 +  tmp-mode = ir_var_auto;
 +  instructions-push_tail(tmp);
 +  ir_dereference_variable *deref_tmp_1
 + = new(ctx) ir_dereference_variable(tmp);
 +  ir_dereference_variable *deref_tmp_2
 + = new(ctx) ir_dereference_variable(tmp);
 +  ir_rvalue *converted_tmp
 + = convert_component(deref_tmp_1, actual-type);
 +  ir_assignment *assignment
 + = new(ctx) ir_assignment(actual, converted_tmp);
 +  post_call_conversions.push_tail(assignment);
 +  actual-replace_with(deref_tmp_2);

[Mesa-dev] [PATCH 3/3] glsl: Perform implicit type conversions on function call out parameters.

2011-08-02 Thread Paul Berry
When an out parameter undergoes an implicit type conversion, we need
to store it in a temporary, and then after the call completes, convert
the resulting value.  In other words, we convert code like the
following:

void f(out int x);
float value;
f(value);

Into IR that's equivalent to this:

void f(out int x);
float value;
int x_converted;
f(x_converted);
value = float(x_converted);

This transformation needs to happen during ast-to-IR convertion (as
opposed to, say, a lowering pass), because it is invalid IR for formal
and actual parameters to have types that don't match.

Fixes piglit tests spec/glsl-1.20/compiler/qualifiers/out-03.vert and
spec/glsl-1.20/execution/qualifiers/vs-out-{01,02,03}.shader_test, and
bug 39651.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=39651
---
 src/glsl/ast_function.cpp |   64 +---
 1 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
index 8bcf48d..f945a94 100644
--- a/src/glsl/ast_function.cpp
+++ b/src/glsl/ast_function.cpp
@@ -134,6 +134,8 @@ match_function_by_name(exec_list *instructions, const char 
*name,
   }
}
 
+   exec_list post_call_conversions;
+
if (sig != NULL) {
   /* Verify that 'out' and 'inout' actual parameters are lvalues.  This
* isn't done in ir_function::matching_signature because that function
@@ -141,6 +143,13 @@ match_function_by_name(exec_list *instructions, const char 
*name,
*
* Also, validate that 'const_in' formal parameters (an extension of our
* IR) correspond to ir_constant actual parameters.
+   *
+   * Also, perform implicit conversion of arguments.  Note: to
+   * implicitly convert out parameters, we need to place them in a
+   * temporary variable, and do the conversion after the call
+   * takes place.  Since we haven't emitted the call yet, we'll
+   * place the post-call conversions in a temporary exec_list, and
+   * emit them later.
*/
   exec_list_iterator actual_iter = actual_parameters-iterator();
   exec_list_iterator formal_iter = sig-parameters.iterator();
@@ -185,8 +194,50 @@ match_function_by_name(exec_list *instructions, const char 
*name,
 }
 
 if (formal-type-is_numeric() || formal-type-is_boolean()) {
-   ir_rvalue *converted = convert_component(actual, formal-type);
-   actual-replace_with(converted);
+switch (formal-mode) {
+case ir_var_in:
+   {
+  ir_rvalue *converted
+ = convert_component(actual, formal-type);
+  actual-replace_with(converted);
+   }
+   break;
+case ir_var_out:
+   if (actual-type != formal-type)
+   {
+  /* Create a temporary variable to hold the out
+   * parameter.  Don't pass a hashtable to clone()
+   * because we don't want this temporary variable to
+   * be in scope.
+   */
+  ir_variable *tmp = formal-clone(ctx, NULL);
+  tmp-mode = ir_var_auto;
+  instructions-push_tail(tmp);
+  ir_dereference_variable *deref_tmp_1
+ = new(ctx) ir_dereference_variable(tmp);
+  ir_dereference_variable *deref_tmp_2
+ = new(ctx) ir_dereference_variable(tmp);
+  ir_rvalue *converted_tmp
+ = convert_component(deref_tmp_1, actual-type);
+  ir_assignment *assignment
+ = new(ctx) ir_assignment(actual, converted_tmp);
+  post_call_conversions.push_tail(assignment);
+  actual-replace_with(deref_tmp_2);
+   }
+   break;
+case ir_var_inout:
+   /* Inout parameters should never require conversion,
+* since that would require an implicit conversion to
+* exist both to and from the formal parameter type,
+* and there are no bidirectional implicit
+* conversions.
+*/
+   assert (actual-type == formal-type);
+   break;
+default:
+   assert (!Illegal formal parameter mode);
+   break;
+}
 }
 
 actual_iter.next();
@@ -196,11 +247,13 @@ match_function_by_name(exec_list *instructions, const 
char *name,
   /* Always insert the call in the instruction stream, and return a deref
* of its return val if it returns a value, since we don't know if
* the rvalue is going to be assigned to anything or not.
+   *
+   * Also insert any out parameter conversions after the call.
*/
   ir_call *call = new(ctx) ir_call(sig, actual_parameters);
+  ir_dereference_variable *deref;
   if 

Re: [Mesa-dev] [PATCH 3/3] glsl: Perform implicit type conversions on function call out parameters.

2011-08-02 Thread Ian Romanick
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 08/02/2011 05:38 PM, Paul Berry wrote:
 When an out parameter undergoes an implicit type conversion, we need
 to store it in a temporary, and then after the call completes, convert
 the resulting value.  In other words, we convert code like the
 following:
 
 void f(out int x);
 float value;
 f(value);
 
 Into IR that's equivalent to this:
 
 void f(out int x);
 float value;
 int x_converted;
 f(x_converted);
 value = float(x_converted);
 
 This transformation needs to happen during ast-to-IR convertion (as
 opposed to, say, a lowering pass), because it is invalid IR for formal
 and actual parameters to have types that don't match.

I want to digest this code a bit more, but I have a couple minor
comments below.

 Fixes piglit tests spec/glsl-1.20/compiler/qualifiers/out-03.vert and
 spec/glsl-1.20/execution/qualifiers/vs-out-{01,02,03}.shader_test, and
 bug 39651.
 
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=39651
 ---
  src/glsl/ast_function.cpp |   64 +---
  1 files changed, 59 insertions(+), 5 deletions(-)
 
 diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
 index 8bcf48d..f945a94 100644
 --- a/src/glsl/ast_function.cpp
 +++ b/src/glsl/ast_function.cpp
 @@ -134,6 +134,8 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
}
 }
  
 +   exec_list post_call_conversions;
 +
 if (sig != NULL) {
/* Verify that 'out' and 'inout' actual parameters are lvalues.  This
 * isn't done in ir_function::matching_signature because that function
 @@ -141,6 +143,13 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
 *
 * Also, validate that 'const_in' formal parameters (an extension of 
 our
 * IR) correspond to ir_constant actual parameters.
 +   *
 +   * Also, perform implicit conversion of arguments.  Note: to
 +   * implicitly convert out parameters, we need to place them in a
 +   * temporary variable, and do the conversion after the call
 +   * takes place.  Since we haven't emitted the call yet, we'll
 +   * place the post-call conversions in a temporary exec_list, and
 +   * emit them later.

It looks like this comment (and the others below) are wrapped to much
less than 80 columns.

 */
exec_list_iterator actual_iter = actual_parameters-iterator();
exec_list_iterator formal_iter = sig-parameters.iterator();
 @@ -185,8 +194,50 @@ match_function_by_name(exec_list *instructions, const 
 char *name,
}
  
if (formal-type-is_numeric() || formal-type-is_boolean()) {
 - ir_rvalue *converted = convert_component(actual, formal-type);
 - actual-replace_with(converted);
 +switch (formal-mode) {
 +case ir_var_in:
 +   {

The { should be on the same line as the case, and emacs got the
indentation wonkey because it's not.

 +  ir_rvalue *converted
 + = convert_component(actual, formal-type);
 +  actual-replace_with(converted);
 +   }
 +   break;
 +case ir_var_out:
 +   if (actual-type != formal-type)
 +   {

The { should be on the same line as the if.

 +  /* Create a temporary variable to hold the out
 +   * parameter.  Don't pass a hashtable to clone()
 +   * because we don't want this temporary variable to
 +   * be in scope.
 +   */
 +  ir_variable *tmp = formal-clone(ctx, NULL);
 +  tmp-mode = ir_var_auto;

ir_var_auto is only used for user-defined variables, and
ir_var_temporary is used for compiler-generated variables.  We'd also
usually do something like:

ir_variable *tmp =
new(mem_ctx) ir_variable(formal-type,
 out_parameter_conversion,
 ir_var_temporary);

Giving it a name like that makes the origin of the variable clear in
shader dumps.  This has proven to be an invaluable debugging tool.

 +  instructions-push_tail(tmp);
 +  ir_dereference_variable *deref_tmp_1
 + = new(ctx) ir_dereference_variable(tmp);
 +  ir_dereference_variable *deref_tmp_2
 + = new(ctx) ir_dereference_variable(tmp);
 +  ir_rvalue *converted_tmp
 + = convert_component(deref_tmp_1, actual-type);
 +  ir_assignment *assignment
 + = new(ctx) ir_assignment(actual, converted_tmp);
 +  post_call_conversions.push_tail(assignment);
 +  actual-replace_with(deref_tmp_2);
 +   }
 +   break;
 +case ir_var_inout:
 +   /* Inout parameters should never require conversion,
 +* since