[Mesa-dev] [PATCH] i965: Work around SIN/COS output range problem.

[Kenneth Graunke]

The SIN and COS instructions on Intel hardware can produce values
slightly outside of the [-1.0, 1.0] range for a small set of values.
Obviously, this can break everyone's expectations about trig functions.

According to an internal presentation, the COS instruction can produce
a value up to 1.000027 for inputs in the range (0.08296, 0.09888).  One
suggested workaround is to multiply by 0.99997, scaling down the
amplitude slightly.  Apparently this also minimizes the error function,
reducing the maximum error from 0.00006 to about 0.00003.

I chose to apply this only when not saturating, as saturate already
clamps to 1.0.  This may or may not be a good idea.

Fixes 16 dEQP precision tests

   dEQP-GLES31.functional.shaders.builtin_functions.precision.
   {cos,sin}.{highp,mediump}_compute.{scalar,vec2,vec4,vec4}.

at the cost of making every sin and cos call more expensive.

Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
---
 src/mesa/drivers/dri/i965/brw_fs_nir.cpp   | 26 ++++++++++++++++++++------
 src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 26 ++++++++++++++++++++------
 2 files changed, 40 insertions(+), 12 deletions(-)

This has been in the Vulkan tree for a while - we needed it to pass the
Vulkan CTS, as it contains these same dEQP tests.

I haven't run shader-db yet, but I don't expect we'll like the results.

The patch is pretty sketchy, too.  I'm sort of tempted to hide it behind
an INTEL_STRICT_CONFORMANCE=1 option, like we had way back in the day...

diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
index cde8f0b..2f56f01 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
@@ -774,15 +774,29 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
       inst->saturate = instr->dest.saturate;
       break;
 
-   case nir_op_fsin:
-      inst = bld.emit(SHADER_OPCODE_SIN, result, op[0]);
-      inst->saturate = instr->dest.saturate;
+   case nir_op_fsin: {
+      fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_F);
+      inst = bld.emit(SHADER_OPCODE_SIN, tmp, op[0]);
+      if (instr->dest.saturate) {
+         inst->dst = result;
+         inst->saturate = true;
+      } else {
+         bld.MUL(result, tmp, brw_imm_f(0.99997));
+      }
       break;
+   }
 
-   case nir_op_fcos:
-      inst = bld.emit(SHADER_OPCODE_COS, result, op[0]);
-      inst->saturate = instr->dest.saturate;
+   case nir_op_fcos: {
+      fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_F);
+      inst = bld.emit(SHADER_OPCODE_COS, tmp, op[0]);
+      if (instr->dest.saturate) {
+         inst->dst = result;
+         inst->saturate = true;
+      } else {
+         bld.MUL(result, tmp, brw_imm_f(0.99997));
+      }
       break;
+   }
 
    case nir_op_fddx:
       if (fs_key->high_quality_derivatives) {
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
index 52977f1..c7a2408 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
@@ -1100,15 +1100,29 @@ vec4_visitor::nir_emit_alu(nir_alu_instr *instr)
       inst->saturate = instr->dest.saturate;
       break;
 
-   case nir_op_fsin:
-      inst = emit_math(SHADER_OPCODE_SIN, dst, op[0]);
-      inst->saturate = instr->dest.saturate;
+   case nir_op_fsin: {
+      src_reg tmp = src_reg(this, glsl_type::vec4_type);
+      inst = emit_math(SHADER_OPCODE_SIN, dst_reg(tmp), op[0]);
+      if (instr->dest.saturate) {
+         inst->dst = dst;
+         inst->saturate = true;
+      } else {
+         emit(MUL(dst, tmp, brw_imm_f(0.99997)));
+      }
       break;
+   }
 
-   case nir_op_fcos:
-      inst = emit_math(SHADER_OPCODE_COS, dst, op[0]);
-      inst->saturate = instr->dest.saturate;
+   case nir_op_fcos: {
+      src_reg tmp = src_reg(this, glsl_type::vec4_type);
+      inst = emit_math(SHADER_OPCODE_COS, dst_reg(tmp), op[0]);
+      if (instr->dest.saturate) {
+         inst->dst = dst;
+         inst->saturate = true;
+      } else {
+         emit(MUL(dst, tmp, brw_imm_f(0.99997)));
+      }
       break;
+   }
 
    case nir_op_idiv:
    case nir_op_udiv:
-- 
2.7.3