On 19.10.2016 23:18, Ian Romanick wrote:
On 10/19/2016 11:45 AM, Nicolai Hähnle wrote:
From: Nicolai Hähnle <[email protected]>
This is required when an out argument involves an array index that is either
a global variable modified by the function or another out argument in the
same function call.
Fixes the shaders/out-parameter-indexing/vs-inout-index-inout-* tests.
--
I'd appreciate if somebody could clarify how much the IR has to be a tree,
and to what extent it is allowed to be a DAG. In particular, for inout
parameters we have the same dereference sub-expression twice. As is, the
patch clones this sub-expression (where the code for passing an inout
parameter into the function is added). That may be unnecessary, but I'm
not sure and I wanted to stay on the safe side.
The only things that can ever be reachable, through any means, more than
once in the IR is an ir_variable (via some sort of ir_dereference) or an
ir_function_signature (via ir_call). This is one of the most important
invariants that ir_validate checks.
This is why lots of places (lowering passes, optimizations, etc.) will
pull some expression from a tree, assign it to a new variable, and use
the new variable instead.
Okay, thanks for the clarification.
Thanks,
Nicolai
---
src/compiler/glsl/opt_function_inlining.cpp | 122 ++++++++++++++++++++++++----
1 file changed, 108 insertions(+), 14 deletions(-)
diff --git a/src/compiler/glsl/opt_function_inlining.cpp
b/src/compiler/glsl/opt_function_inlining.cpp
index 83534bf..eee8fd7 100644
--- a/src/compiler/glsl/opt_function_inlining.cpp
+++ b/src/compiler/glsl/opt_function_inlining.cpp
@@ -88,28 +88,87 @@ replace_return_with_assignment(ir_instruction *ir, void
*data)
} else {
/* un-valued return has to be the last return, or we shouldn't
* have reached here. (see can_inline()).
*/
assert(ret->next->is_tail_sentinel());
ret->remove();
}
}
}
+/* Save the given lvalue before the given instruction.
+ *
+ * This is done by adding temporary variables into which the current value
+ * of any array indices are saved, and then reconstructing a dereference chain
+ * that uses those temporary variables.
+ *
+ * This function returns the original lvalue if there were no array indices
+ * to save.
+ */
+static ir_rvalue *
+save_lvalue(void *ctx, ir_rvalue *lvalue, ir_instruction *insert_before)
+{
This should use an ir_hierarchical_visitor. There's a pretty
significant bug at the bottom, and using the visitor will fix that.
I considered that, but I don't think the hierarchical visitor makes
sense. An ir_visitor perhaps, but anyway this should be discussed below...
+ if (ir_dereference_record *deref_record = lvalue->as_dereference_record()) {
+ ir_rvalue *saved_record;
+
+ saved_record = save_lvalue(ctx, deref_record->record, insert_before);
+ if (saved_record == deref_record->record)
+ return lvalue;
+
+ return new(ctx) ir_dereference_record(saved_record, deref_record->field);
This could be simplified as:
lvalue->record = save_lvalue(ctx, deref_record->record,
insert_before);
return lvalue;
Will do.
+ }
+
+ if (ir_swizzle *swizzle = lvalue->as_swizzle()) {
+ ir_rvalue *saved_val;
+
+ saved_val = save_lvalue(ctx, swizzle->val, insert_before);
+ if (saved_val == swizzle->val)
+ return lvalue;
+
+ return new(ctx) ir_swizzle(saved_val, swizzle->mask);
This could be simplified as:
lvalue->val = save_lvalue(ctx, swizzle->val, insert_before);
return lvalue;
Will do.
+ }
+
+ if (ir_dereference_array *deref_array = lvalue->as_dereference_array()) {
+ ir_rvalue *saved_array;
+ ir_variable *index;
+ ir_assignment *assignment;
+
+ saved_array = save_lvalue(ctx, deref_array->array, insert_before);
+ if (saved_array == deref_array->array &&
+ deref_array->array_index->ir_type == ir_type_constant)
+ return lvalue;
+
+ index = new(ctx) ir_variable(deref_array->array_index->type,
"saved_idx", ir_var_temporary);
+ insert_before->insert_before(index);
+
+ assignment = new(ctx) ir_assignment(new(ctx)
ir_dereference_variable(index),
+ deref_array->array_index, 0);
+ insert_before->insert_before(assignment);
+
+ return new(ctx) ir_dereference_array(saved_array,
+ new(ctx)
ir_dereference_variable(index));
+ }
+
+ assert(lvalue->ir_type == ir_type_dereference_variable);
I think the ir_dereference_array and ir_dereference_variable handling is
not quite the right approach. Basically you want to replace any
expression involving x with an identical expression involving in new
variable y and an assignment of x to y. Right? I think your
implementation won't catch cases like
foo(a[x+y], x, y);
Well, that should be semantically equivalent to
tmp = x+y;
foo(a[tmp], x, y);
which is precisely what this code does, isn't it?
Also, consider more complex examples like:
foo(a[bar(...)]);
which should behave like
tmp = bar(...);
foo(a[tmp]);
Replacing variables isn't sufficient there. Part of what this patch does
is ensure that bar is called (a) before the call to foo instead of
afterwards, and (b) only once even when it's an inout-parameter.
Maybe there's something else I missed, but I still believe the current
approach is right. About using an ir_hierarchical_visitor, I personally
feel that it would obfuscate what's going on because I actually _don't_
want to visit everything in the tree recursively.
Thanks,
Nicolai
I think the easiest thing to do is make an ir_hierarchical visitor that
replaces the var of every ir_dereference_variable see inside the
array_index of an ir_dereference_array with a new variable and an
assignment. Just increment / decrement a count in
ir_visit_enter(ir_dereference_array*) and
ir_visit_leave(ir_dereference_array*). If count is non-zero in
ir_visit(ir_dereference_variable*), do the replacement.
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
