From 8d10a6835f32ec677759f9d37e08dcc95aaab17a Mon Sep 17 00:00:00 2001
From: Timothy Arceri <tarceri@itsqueeze.com>
Date: Thu, 3 Nov 2022 12:59:20 +1100
Subject: [PATCH] glsl: dont create temps for builtin function inputs

It's not valid to be copying input variables to temps when
inlining atomic memory, interpolateAt functions, etc. We got away
with this previously because tree grafting would clean up the
mess but we shouldn't depend on an optimisation to clean up
invalid IR. Also I hope to remove tree grafting in a follow up
merge request.

Reviewed-by: Emma Anholt <emma@anholt.net>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/19890>
---
 src/compiler/glsl/opt_function_inlining.cpp   | 30 +++++++++++++++----
 .../glsl/tests/lower_precision_test.py        |  6 ++--
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/src/compiler/glsl/opt_function_inlining.cpp b/src/compiler/glsl/opt_function_inlining.cpp
index d27944d913b..44a18cfd8d8 100644
--- a/src/compiler/glsl/opt_function_inlining.cpp
+++ b/src/compiler/glsl/opt_function_inlining.cpp
@@ -133,15 +133,31 @@ ir_save_lvalue_visitor::visit_enter(ir_dereference_array *deref)
 }
 
 static bool
-should_replace_variable(ir_variable *sig_param, ir_rvalue *param) {
+should_replace_variable(ir_variable *sig_param, ir_rvalue *param,
+                        bool is_builtin) {
+
+   if (sig_param->data.mode != ir_var_function_in &&
+       sig_param->data.mode != ir_var_const_in)
+      return false;
+
+   /* SSBO and shared vars might be passed to a built-in such as an atomic
+    * memory function, where copying these to a temp before passing to the
+    * atomic function is not valid so we must replace these instead. Also,
+    * shader inputs for interpolateAt funtions also need to be replaced.
+    *
+    * Our builtins should always use temps and not the inputs themselves to
+    * store temporay values so just checking is_builtin rather than string
+    * comparing the function name for e.g atomic* should always be safe.
+    */
+   if (is_builtin)
+      return true;
+
    /* For opaque types, we want the inlined variable references
     * referencing the passed in variable, since that will have
     * the location information, which an assignment of an opaque
     * variable wouldn't.
     */
-   return sig_param->type->contains_opaque() &&
-          param->is_dereference() &&
-          sig_param->data.mode == ir_var_function_in;
+   return sig_param->type->contains_opaque();
 }
 
 void
@@ -168,7 +184,8 @@ ir_call::generate_inline(ir_instruction *next_ir)
       ir_rvalue *param = (ir_rvalue *) actual_node;
 
       /* Generate a new variable for the parameter. */
-      if (should_replace_variable(sig_param, param)) {
+      if (should_replace_variable(sig_param, param,
+                                  this->callee->is_builtin())) {
          /* Actual replacement happens below */
 	 parameters[i] = NULL;
       } else {
@@ -251,7 +268,8 @@ ir_call::generate_inline(ir_instruction *next_ir)
       ir_rvalue *const param = (ir_rvalue *) actual_node;
       ir_variable *sig_param = (ir_variable *) formal_node;
 
-      if (should_replace_variable(sig_param, param)) {
+      if (should_replace_variable(sig_param, param,
+                                  this->callee->is_builtin())) {
          do_variable_replacement(&new_instructions, sig_param, param);
       }
    }
diff --git a/src/compiler/glsl/tests/lower_precision_test.py b/src/compiler/glsl/tests/lower_precision_test.py
index 999c6267af3..21fc572187f 100644
--- a/src/compiler/glsl/tests/lower_precision_test.py
+++ b/src/compiler/glsl/tests/lower_precision_test.py
@@ -1268,7 +1268,7 @@ TESTS = [
                  color2 = y + 1;
          }
          """,
-         r'assign  \(x\) \(var_ref x@2\)  \(expression float f162f'),
+         r'assign  \(x\) \(var_ref compiler_temp@2\)  \(expression uint bitcast_f2u \(expression float f162f'),
     Test("ldexp",
          """
          #version 310 es
@@ -1301,7 +1301,7 @@ TESTS = [
                  color *= carry;
          }
          """,
-         r'expression uint \+ \(var_ref x\) \(var_ref y'),
+         r'expression uint \+ \(expression uint u2u \(expression uint16_t \* \(expression uint16_t u2ump \(var_ref x\) \) \(constant uint16_t \(2\)\) \) \) \(expression uint u2u \(expression uint16_t \* \(expression uint16_t u2ump \(var_ref y'),
     Test("usubBorrow",
          """
          #version 310 es
@@ -1318,7 +1318,7 @@ TESTS = [
                  color *= borrow;
          }
          """,
-         r'expression uint \- \(var_ref x\) \(var_ref y'),
+         r'expression uint \- \(expression uint u2u \(expression uint16_t \* \(expression uint16_t u2ump \(var_ref x\) \) \(constant uint16_t \(2\)\) \) \) \(expression uint u2u \(expression uint16_t \* \(expression uint16_t u2ump \(var_ref y'),
     Test("imulExtended",
          """
          #version 310 es