[Mesa-dev] [PATCH] r600/sb: Check whether optimizations would result in reladdr conflict

[Gert Wollny]

Two cases are handled with this patch:

1. If copy propagation tries to eliminated a move from a relative
   array access then it could optimize

     MOV R1, ARRAY[RELADDR_1]
     MOV R2, ARRAY[RELADDR_2]
     OP2 R3, R1 R2

   into

     OP2 R3, ARRAY[RELADDR_1], ARRAY[RELADDR_2]

   which is forbidden, because there is only one address register available.

2. When MULADD(x,a,MUL(x,c)) is handled

      MUL TMP, R1, ARRAY[RELADDR_1]
      MULLADD R3, R1, ARRAY[RELADDR_2], TMP

   by folding this into

      ADD TMP, ARRAY[RELADDR_2], ARRAY[RELADDR_1]
      MUL R3, R1, TMP
   
   which is also forbitten. 

Test for these cases and reject the optimization if a forbidden combination
of relative access would be created.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103142
Signed-off-by: Gert Wollny <gw.fossdev at gmail.com>
---
I folded both cases into one patch because they both are required to fix #103142.
There may be other instances of this type of bug be lurking, but since this bug
only triggers an assert in debug builds and is already worked around in release
builds I think when lending this patch the bug can be closed.

Best,
Gert

PS: I have no git write access.

 src/gallium/drivers/r600/sb/sb_expr.cpp     | 19 ++++++++++++-----
 src/gallium/drivers/r600/sb/sb_ir.h         |  6 ++++++
 src/gallium/drivers/r600/sb/sb_valtable.cpp | 33 +++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/src/gallium/drivers/r600/sb/sb_expr.cpp b/src/gallium/drivers/r600/sb/sb_expr.cpp
index 7d43ef1d1d..e339651f62 100644
--- a/src/gallium/drivers/r600/sb/sb_expr.cpp
+++ b/src/gallium/drivers/r600/sb/sb_expr.cpp
@@ -412,7 +412,8 @@ bool expr_handler::fold_alu_op1(alu_node& n) {
 				n.bc.op == ALU_OP1_MOVA_GPR_INT)
 				&& n.bc.clamp == 0 && n.bc.omod == 0
 				&& n.bc.src[0].abs == 0 && n.bc.src[0].neg == 0 &&
-				n.src.size() == 1 /* RIM/SIM can be appended as additional values */) {
+				n.src.size() == 1 /* RIM/SIM can be appended as additional values */
+				&& n.dst[0]->no_reladdr_conflict_with(v0)) {
 			assign_source(n.dst[0], v0);
 			return true;
 		}
@@ -1027,10 +1028,18 @@ bool expr_handler::fold_alu_op3(alu_node& n) {
 				es1 = 1;
 			}
 
-			if (es0 != -1) {
-				value *va0 = es0 == 0 ? v1 : v0;
-				value *va1 = es1 == 0 ? mv1 : mv0;
-
+			value *va0 = es0 == 0 ? v1 : v0;
+			value *va1 = es1 == 0 ? mv1 : mv0;
+
+			/* Don't fold if no equal multipliers were found.
+			 * Also don#t fold if the operands of the to be created ADD are both
+			 * relatively accessed with different AR values because that would
+			 * create impossible code.
+			 */
+			if (es0 != -1 &&
+			    (!va0->is_rel() || !va1->is_rel() ||
+			     (va0->rel == va1->rel))) {
+
 				alu_node *add = sh.create_alu();
 				add->bc.set_op(ALU_OP2_ADD);
 
diff --git a/src/gallium/drivers/r600/sb/sb_ir.h b/src/gallium/drivers/r600/sb/sb_ir.h
index bee947504e..6acc2b42ae 100644
--- a/src/gallium/drivers/r600/sb/sb_ir.h
+++ b/src/gallium/drivers/r600/sb/sb_ir.h
@@ -612,6 +612,12 @@ public:
 		}
 	}
 
+	/* Check whether copy-propagation of src into this would create an access
+	 * conflict with relative addressing, i.e. an operation that tries to access
+	 * array elements with different address register values.
+	 */
+	bool no_reladdr_conflict_with(value *src);
+
 	val_set interferences;
 	unsigned uid;
 };
diff --git a/src/gallium/drivers/r600/sb/sb_valtable.cpp b/src/gallium/drivers/r600/sb/sb_valtable.cpp
index 41cfbf0946..70e6079b2c 100644
--- a/src/gallium/drivers/r600/sb/sb_valtable.cpp
+++ b/src/gallium/drivers/r600/sb/sb_valtable.cpp
@@ -295,6 +295,38 @@ void value::delete_uses() {
 	uses.erase(uses.begin(), uses.end());
 }
 
+bool value::no_reladdr_conflict_with(value *src)
+{
+	/*  if the register is not relative, it can't create an relative access conflict */
+	if (!src->is_rel())
+		return true;
+
+	/* If the destination is AR then we accept the copy propagation, because the
+	 * scheduler actually re-creates the address loading operation and will
+	 * signal interference if there is an address register load and it will fail
+	 * because of this.
+	 */
+	if (gvalue()->is_AR())
+		return true;
+
+	/* For all nodes that use this value test whether the operation uses another
+	 * relative access with a different address value. If found, signal conflict.
+	 */
+	for (uselist::const_iterator N = uses.begin(); N != uses.end(); ++N) {
+		for (vvec::const_iterator V = (*N)->src.begin(); V != (*N)->src.end(); ++V) {
+			value *v = (*V)->gvalue();
+			if (v && v != src && v->is_rel() && v->rel != src->rel)
+				return false;
+		}
+		for (vvec::const_iterator V = (*N)->dst.begin(); V != (*N)->dst.end(); ++V) {
+			value *v = (*V)->gvalue();
+			if (v && v != src && v->is_rel() && (v->rel != src->rel))
+				return false;
+		}
+	}
+	return true;
+}
+
 void ra_constraint::update_values() {
 	for (vvec::iterator I = values.begin(), E = values.end(); I != E; ++I) {
 		assert(!(*I)->constraint);
-- 
2.13.6