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

[Gert Wollny]

v2: * Check whether the node src and dst registers are NULL before using 
      them.
    * fix a type in the commit message. 

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 forbidden.

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>
---
 src/gallium/drivers/r600/sb/sb_expr.cpp     | 17 ++++++++++----
 src/gallium/drivers/r600/sb/sb_ir.h         |  6 +++++
 src/gallium/drivers/r600/sb/sb_valtable.cpp | 36 +++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/src/gallium/drivers/r600/sb/sb_expr.cpp b/src/gallium/drivers/r600/sb/sb_expr.cpp
index 7d43ef1d1d..1df78da660 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,9 +1028,17 @@ 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..74a15900d1 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..b4e276a02d 100644
--- a/src/gallium/drivers/r600/sb/sb_valtable.cpp
+++ b/src/gallium/drivers/r600/sb/sb_valtable.cpp
@@ -295,6 +295,42 @@ 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) {
+			if (*V) {
+				value *v = (*V)->gvalue();
+				if (v != src && v->is_rel() && v->rel != src->rel)
+					return false;
+			}
+		}
+		for (vvec::const_iterator V = (*N)->dst.begin(); V != (*N)->dst.end(); ++V) {
+			if (*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