Re: [Mesa-dev] [PATCH] glsl: correctly handle inout parameters post function inlining
Sounds good to me, go ahead and push the new version! On Fri, May 6, 2016 at 10:44 AM, Juan A. Suarez Romero wrote: > On Fri, 2016-05-06 at 10:39 -0400, Lars Hamre wrote: >> Hi Juan, >> >> Sorry I missed that. >> >> It looks like your patch doesn't fix the out parameter indexing for: >> vs-inout-index-inout-mat2-col >> vs-inout-index-inout-vec4-array >> >> I was able to extend your patch to get these tests passing by: >> - if the ir_array->array was a dereferenced_array, copy propagate >> it's index >> - if the ir->ir_type == ir_type_swizzle, if it's val was a >> dereferenced_array, >> copying propagate it's index > > That's great! Precisely, I have here a version that fixes those two > test by doing exactly that :D > > > I can push a new version with that change, or if you prefer doing it > yourself, just let me know > > > J.A. > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: correctly handle inout parameters post function inlining
On Fri, 2016-05-06 at 10:39 -0400, Lars Hamre wrote: > Hi Juan, > > Sorry I missed that. > > It looks like your patch doesn't fix the out parameter indexing for: > vs-inout-index-inout-mat2-col > vs-inout-index-inout-vec4-array > > I was able to extend your patch to get these tests passing by: > - if the ir_array->array was a dereferenced_array, copy propagate > it's index > - if the ir->ir_type == ir_type_swizzle, if it's val was a > dereferenced_array, > copying propagate it's index That's great! Precisely, I have here a version that fixes those two test by doing exactly that :D I can push a new version with that change, or if you prefer doing it yourself, just let me know J.A. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: correctly handle inout parameters post function inlining
I think you want a loop there, peeling off all array dereferences, due to AoA (and maybe throw in some piglit tests to cover that). [Assuming AoA's can be function arguments... haven't checked the spec.] On Fri, May 6, 2016 at 10:39 AM, Lars Hamre wrote: > Hi Juan, > > Sorry I missed that. > > It looks like your patch doesn't fix the out parameter indexing for: > vs-inout-index-inout-mat2-col > vs-inout-index-inout-vec4-array > > I was able to extend your patch to get these tests passing by: > - if the ir_array->array was a dereferenced_array, copy propagate it's index > - if the ir->ir_type == ir_type_swizzle, if it's val was a > dereferenced_array, > copying propagate it's index > > I would rather move ahead with your patch as mine seems quite > "hacky" in comparison :) > > Regards, > Lars Hamre > > On Fri, May 6, 2016 at 9:53 AM, Juan A. Suarez Romero > wrote: >> On Fri, 2016-05-06 at 08:49 -0400, Lars Hamre wrote: >>> Inout parameters which depended on other inout parameters >>> where not assigned in the correct order. >>> >>> Fixes the following piglit tests in shaders/out-parameter-indexing: >>> vs-inout-index-inout-float-array >>> vs-inout-index-inout-mat2-col >>> vs-inout-index-inout-mat2-row >>> vs-inout-index-inout-vec4 >>> vs-inout-index-inout-vec4-array >>> vs-inout-index-inout-vec4-array-element >> >> >> Some days ago I sent a patch that also fixes some of these tests in a >> different way. >> >> https://lists.freedesktop.org/archives/mesa-dev/2016-May/115621.html >> >> >> >> J.A. >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: correctly handle inout parameters post function inlining
Hi Juan, Sorry I missed that. It looks like your patch doesn't fix the out parameter indexing for: vs-inout-index-inout-mat2-col vs-inout-index-inout-vec4-array I was able to extend your patch to get these tests passing by: - if the ir_array->array was a dereferenced_array, copy propagate it's index - if the ir->ir_type == ir_type_swizzle, if it's val was a dereferenced_array, copying propagate it's index I would rather move ahead with your patch as mine seems quite "hacky" in comparison :) Regards, Lars Hamre On Fri, May 6, 2016 at 9:53 AM, Juan A. Suarez Romero wrote: > On Fri, 2016-05-06 at 08:49 -0400, Lars Hamre wrote: >> Inout parameters which depended on other inout parameters >> where not assigned in the correct order. >> >> Fixes the following piglit tests in shaders/out-parameter-indexing: >> vs-inout-index-inout-float-array >> vs-inout-index-inout-mat2-col >> vs-inout-index-inout-mat2-row >> vs-inout-index-inout-vec4 >> vs-inout-index-inout-vec4-array >> vs-inout-index-inout-vec4-array-element > > > Some days ago I sent a patch that also fixes some of these tests in a > different way. > > https://lists.freedesktop.org/archives/mesa-dev/2016-May/115621.html > > > > J.A. > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: correctly handle inout parameters post function inlining
On Fri, 2016-05-06 at 08:49 -0400, Lars Hamre wrote: > Inout parameters which depended on other inout parameters > where not assigned in the correct order. > > Fixes the following piglit tests in shaders/out-parameter-indexing: > vs-inout-index-inout-float-array > vs-inout-index-inout-mat2-col > vs-inout-index-inout-mat2-row > vs-inout-index-inout-vec4 > vs-inout-index-inout-vec4-array > vs-inout-index-inout-vec4-array-element Some days ago I sent a patch that also fixes some of these tests in a different way. https://lists.freedesktop.org/archives/mesa-dev/2016-May/115621.html J.A. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl: correctly handle inout parameters post function inlining
Inout parameters which depended on other inout parameters
where not assigned in the correct order.
Fixes the following piglit tests in shaders/out-parameter-indexing:
vs-inout-index-inout-float-array
vs-inout-index-inout-mat2-col
vs-inout-index-inout-mat2-row
vs-inout-index-inout-vec4
vs-inout-index-inout-vec4-array
vs-inout-index-inout-vec4-array-element
Signed-off-by: Lars Hamre
---
NOTES:
- Someone with access will need to commit this post
review process.
- Not sure if this is the "mesa" approach to this
problem, feedback is appreciated.
src/compiler/glsl/opt_function_inlining.cpp | 80 ++---
1 file changed, 73 insertions(+), 7 deletions(-)
diff --git a/src/compiler/glsl/opt_function_inlining.cpp
b/src/compiler/glsl/opt_function_inlining.cpp
index 19f5fae..a5e7ea6 100644
--- a/src/compiler/glsl/opt_function_inlining.cpp
+++ b/src/compiler/glsl/opt_function_inlining.cpp
@@ -95,6 +95,54 @@ replace_return_with_assignment(ir_instruction *ir, void
*data)
}
}
+static void
+gather_inout_vars_from_params(void *ctx,
+ exec_list *actual_params,
+ exec_list *callee_params,
+ exec_list *inout_vars)
+{
+ foreach_two_lists(formal_node, callee_params,
+ actual_node, actual_params) {
+ ir_rvalue *const param = (ir_rvalue *)actual_node;
+ const ir_variable *const sig_param = (ir_variable *)formal_node;
+
+ if (sig_param->data.mode == ir_var_function_inout &&
+param->ir_type == ir_type_dereference_variable) {
+ inout_vars->push_tail(sig_param->clone(ctx, NULL));
+ }
+ }
+}
+
+static bool
+rvalue_depends_on_inout_var_helper(ir_rvalue *rv, exec_list *inout_vars)
+{
+ if (rv->ir_type == ir_type_dereference_array) {
+ ir_dereference_array *deref_arr = (ir_dereference_array *)rv;
+ return (rvalue_depends_on_inout_var_helper(deref_arr->array, inout_vars)
||
+ rvalue_depends_on_inout_var_helper(deref_arr->array_index,
inout_vars));
+ } else if (rv->ir_type == ir_type_swizzle) {
+ return rvalue_depends_on_inout_var_helper(((ir_swizzle *)rv)->val,
inout_vars);
+ } else if (rv->ir_type == ir_type_dereference_variable) {
+ const char *var_name = ((ir_dereference_variable *)rv)->var->name;
+ foreach_in_list(ir_variable, inout_var, inout_vars) {
+ if (!strcmp(var_name, inout_var->name)) {
+return true;
+ }
+ }
+ }
+ return false;
+}
+
+static bool
+rvalue_depends_on_inout_var(ir_rvalue *rv, exec_list *inout_vars)
+{
+ if (rv->ir_type == ir_type_dereference_array ||
+ rv->ir_type == ir_type_swizzle) {
+ return rvalue_depends_on_inout_var_helper(rv, inout_vars);
+ }
+ return false;
+}
+
void
ir_call::generate_inline(ir_instruction *next_ir)
{
@@ -185,7 +233,14 @@ ir_call::generate_inline(ir_instruction *next_ir)
/* Copy back the value of any 'out' parameters from the function body
* variables to our own.
*/
+
+ exec_list inout_vars;
+ gather_inout_vars_from_params(ctx, &this->actual_parameters,
+ &this->callee->parameters,
+ &inout_vars);
+
i = 0;
+ ir_instruction *last_non_dependent_inout_assigned = next_ir;
foreach_two_lists(formal_node, &this->callee->parameters,
actual_node, &this->actual_parameters) {
ir_rvalue *const param = (ir_rvalue *) actual_node;
@@ -194,12 +249,24 @@ ir_call::generate_inline(ir_instruction *next_ir)
/* Move our param variable into the actual param if it's an 'out' type.
*/
if (parameters[i] && (sig_param->data.mode == ir_var_function_out ||
sig_param->data.mode == ir_var_function_inout)) {
-ir_assignment *assign;
-
-assign = new(ctx) ir_assignment(param->clone(ctx, NULL)->as_rvalue(),
-new(ctx)
ir_dereference_variable(parameters[i]),
-NULL);
-next_ir->insert_before(assign);
+ ir_assignment *assign;
+
+ assign = new(ctx) ir_assignment(param->clone(ctx, NULL)->as_rvalue(),
+ new(ctx)
ir_dereference_variable(parameters[i]),
+ NULL);
+
+ if (sig_param->data.mode == ir_var_function_inout &&
+ rvalue_depends_on_inout_var(param, &inout_vars)) {
+/*
+ * Insert assignment before the others if it depends on another
+ * inout parameter.
+ */
+last_non_dependent_inout_assigned->insert_before(assign);
+ } else {
+/* Otherwise insert assignment before the next_ir */
+next_ir->insert_before(assign);
+last_non_dependent_inout_assigned = assign;
+ }
}
++i;
@@ -210,7 +277,6 @@ ir_call::generate_inline(ir_instruction
