Hi Vadim,
I've been looking with Glenn's help into a bug in sb for a couple of
weeks now triggered by a change in how GLSL generates switch
statements.
I understand you probably aren't too interested in r600g but I believe
I'm hitting a design level problem and I would like some advice.
So it appears that GLSL can create loops that don't repeat for switch
statements, and it appears SB wasn't ready to handle such a thing.
sb has the ->is_loop() and it just checks !repeats.empty(), so this
meant in the finalizer code we'd fall into the if statement which
would then assert.
I hacked/fixed (more hacked), this in 7b0067d23a6f64cf83c42e7f11b2cd4100c569fe
which attempts to detect single pass loops and handle things that way.
However this lead to stack depth calculations being incorrectly done,
so I moved the single loop detect into the is_loop check, (see
attached patch).
This fixes the rendering in some places, but lead to a regression in
tests/shaders/glsl-vs-continue-in-switch-in-do-while.shader_test
error at : PHI t76||FP@R3.x, t128||FP@R3.x, t115||FP@R3.x,
t102||FP@R3.x, t89||FP@R3.x : expected
operand value t115||FP@R3.x, gpr contains t17||FP@R3.x
error at : PHI t76||FP@R3.x, t128||FP@R3.x, t115||FP@R3.x,
t102||FP@R3.x, t89||FP@R3.x : expected
operand value t102||FP@R3.x, gpr contains t17||FP@R3.x
Now Glenn suspected this was due to the is_loop check in
sb_shader.cpp:create_bbs,
and changing that check to only detect repeating loops removes that issue,
but introduces stack sizing issues again, resulting in lockups/random rendering.
So I just want to ask had you considered single loops with an always
break in sb design,
and perhaps some idea where things are going so wrong with the
register alloc above.
I suspect I'll keep digging into this, but its getting to the edges of
the brain space/time I can find!
Dave.
From 170184b712d9596f761acdee2c7cff2a2792d937 Mon Sep 17 00:00:00 2001
From: Dave Airlie <airl...@redhat.com>
Date: Wed, 3 Dec 2014 13:05:18 +1000
Subject: [PATCH] r600g/sb: detect empty once iterated loops
Since GLSL changed to using loops for switches,
we've hit a bug in sb with single execution loops,
I previously attempted to fix this by changing where
we detected loops, however this isn't good enough
as SB gets the stack sizing wrong.
Fix this by checking inside the is_loop for
single execution loops.
This should fix lockups on rv635 and misrenderings
on cayman since the first fix:
7b0067d23a6f64cf83c42e7f11b2cd4100c569fe
fix issues cause by GLSL switching to loops for switch
Signed-off-by: Dave Airlie <airl...@redhat.com>
---
src/gallium/drivers/r600/sb/sb_bc_finalize.cpp | 22 +++++++------------
src/gallium/drivers/r600/sb/sb_ir.cpp | 30 ++++++++++++++++++++++++++
src/gallium/drivers/r600/sb/sb_ir.h | 3 +--
3 files changed, 39 insertions(+), 16 deletions(-)
diff --git a/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp b/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
index 0fa0910..56189c9 100644
--- a/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
+++ b/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
@@ -46,19 +46,7 @@ int bc_finalizer::run() {
for (regions_vec::reverse_iterator I = rv.rbegin(), E = rv.rend(); I != E;
++I) {
region_node *r = *I;
- bool is_if = false;
- assert(r);
-
- assert(r->first);
- if (r->first->is_container()) {
- container_node *repdep1 = static_cast<container_node*>(r->first);
- assert(repdep1->is_depart() || repdep1->is_repeat());
- if_node *n_if = static_cast<if_node*>(repdep1->first);
- if (n_if && n_if->is_if())
- is_if = true;
- }
-
- if (is_if)
+ if (!r->is_loop())
finalize_if(r);
else
finalize_loop(r);
@@ -121,7 +109,13 @@ void bc_finalizer::finalize_loop(region_node* r) {
cf_node *loop_end = sh.create_cf(CF_OP_LOOP_END);
bool has_instr = false;
- if (!r->is_loop()) {
+ /*
+ * if we have repeats then we have instructions,
+ * if we have no repeats for a single loops,
+ * check if there are any instructions in the depart
+ * nodes.
+ */
+ if (r->repeats.empty()) {
for (depart_vec::iterator I = r->departs.begin(), E = r->departs.end();
I != E; ++I) {
depart_node *dep = *I;
diff --git a/src/gallium/drivers/r600/sb/sb_ir.cpp b/src/gallium/drivers/r600/sb/sb_ir.cpp
index 5226893..a4c4e83 100644
--- a/src/gallium/drivers/r600/sb/sb_ir.cpp
+++ b/src/gallium/drivers/r600/sb/sb_ir.cpp
@@ -485,6 +485,36 @@ void container_node::collect_stats(node_stats& s) {
}
}
+bool region_node::is_loop() {
+
+ if (!repeats.empty())
+ return true;
+
+ /*
+ * single pass loops have no repeats, however we need to detect
+ * them as loops.
+ */
+
+ /* if we have no first in the region then it can't be a loop. */
+ if (!first)
+ return false;
+
+ /*
+ * if the first is a container, see if it has an if node,
+ * if nodes aren't loops, if there is no if node,
+ * then this is a single pass loops.
+ */
+ if (first->is_container()) {
+ container_node *repdep1 = static_cast<container_node*>(first);
+ assert(repdep1->is_depart() || repdep1->is_repeat());
+ if_node *n_if = static_cast<if_node*>(repdep1->first);
+ if (!n_if || !n_if->is_if())
+ return true;
+ }
+ return false;
+}
+
+
void region_node::expand_depart(depart_node *d) {
depart_vec::iterator I = departs.begin() + d->dep_id, E;
I = departs.erase(I);
diff --git a/src/gallium/drivers/r600/sb/sb_ir.h b/src/gallium/drivers/r600/sb/sb_ir.h
index 85c3d06..7d1db3f 100644
--- a/src/gallium/drivers/r600/sb/sb_ir.h
+++ b/src/gallium/drivers/r600/sb/sb_ir.h
@@ -1106,8 +1106,7 @@ public:
unsigned dep_count() { return departs.size(); }
unsigned rep_count() { return repeats.size() + 1; }
- bool is_loop() { return !repeats.empty(); }
-
+ bool is_loop();
container_node* get_entry_code_location() {
node *p = first;
while (p && (p->is_depart() || p->is_repeat()))
--
1.9.3
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
