Re: [Mesa-dev] [PATCH v2 4/4] spirv: Accept doubles in FaceForward, Reflect and Refract

2018-04-17 Thread Jason Ekstrand
On Wed, Mar 21, 2018 at 12:34 PM, Neil Roberts  wrote:

> The SPIR-V spec doesn’t specify a size requirement for these and the
> equivalent functions in the GLSL spec have explicit alternatives for
> doubles. Refract is a little bit more complicated due to the fact that
> the final argument is always supposed to be a scalar 32- or 16- bit
> float regardless of the other operands. However in practice it seems
> there is a bug in glslang that makes it convert the argument to 64-bit
> if you actually try to pass it a 32-bit value while the other
> arguments are 64-bit. This adds an optional conversion of the final
> argument in order to support any type.
>
> These have been tested against the automatically generated tests of
> glsl-4.00/execution/built-in-functions using the ARB_gl_spirv branch
> which tests it with quite a large range of combinations.
>
> The issue with glslang has been filed here:
> https://github.com/KhronosGroup/glslang/issues/1279
>
> v2: Convert the eta operand of Refract from any size in order to make
> it eventually cope with 16-bit floats.
> ---
>  src/compiler/spirv/vtn_glsl450.c | 22 ++
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/src/compiler/spirv/vtn_glsl450.c b/src/compiler/spirv/vtn_
> glsl450.c
> index 50783fbfb4d..0cabedf741d 100644
> --- a/src/compiler/spirv/vtn_glsl450.c
> +++ b/src/compiler/spirv/vtn_glsl450.c
> @@ -628,14 +628,14 @@ handle_glsl450_alu(struct vtn_builder *b, enum
> GLSLstd450 entrypoint,
> case GLSLstd450FaceForward:
>val->ssa->def =
>   nir_bcsel(nb, nir_flt(nb, nir_fdot(nb, src[2], src[1]),
> -   nir_imm_float(nb, 0.0)),
> +   NIR_IMM_FP(nb, 0.0)),
> src[0], nir_fneg(nb, src[0]));
>return;
>
> case GLSLstd450Reflect:
>/* I - 2 * dot(N, I) * N */
>val->ssa->def =
> - nir_fsub(nb, src[0], nir_fmul(nb, nir_imm_float(nb, 2.0),
> + nir_fsub(nb, src[0], nir_fmul(nb, NIR_IMM_FP(nb, 2.0),
>nir_fmul(nb, nir_fdot(nb, src[0], src[1]),
> src[1])));
>return;
> @@ -645,8 +645,22 @@ handle_glsl450_alu(struct vtn_builder *b, enum
> GLSLstd450 entrypoint,
>nir_ssa_def *N = src[1];
>nir_ssa_def *eta = src[2];
>nir_ssa_def *n_dot_i = nir_fdot(nb, N, I);
> -  nir_ssa_def *one = nir_imm_float(nb, 1.0);
> -  nir_ssa_def *zero = nir_imm_float(nb, 0.0);
> +  nir_ssa_def *one = NIR_IMM_FP(nb, 1.0);
> +  nir_ssa_def *zero = NIR_IMM_FP(nb, 0.0);
> +  /* According to the SPIR-V and GLSL specs, eta is always a float
> +   * regardless of the type of the other operands. However in
> practice it
> +   * seems that if you try to pass it a float then glslang will just
> +   * promote it to a double and generate invalid SPIR-V. In order to
> +   * support a hypothetical fixed version of glslang we’ll promote
> eta to
> +   * double if the other operands are double also.
> +   */
>

That's a bit ugly.  Seems like the right thing to do though

Reviewed-by: Jason Ekstrand 


> +  if (I->bit_size != eta->bit_size) {
> + nir_op conversion_op =
> +nir_type_conversion_op(nir_type_float | eta->bit_size,
> +   nir_type_float | I->bit_size,
> +   nir_rounding_mode_undef);
> + eta = nir_build_alu(nb, conversion_op, eta, NULL, NULL, NULL);
> +  }
>/* k = 1.0 - eta * eta * (1.0 - dot(N, I) * dot(N, I)) */
>nir_ssa_def *k =
>   nir_fsub(nb, one, nir_fmul(nb, eta, nir_fmul(nb, eta,
> --
> 2.14.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


[Mesa-dev] [PATCH v2 4/4] spirv: Accept doubles in FaceForward, Reflect and Refract

2018-03-21 Thread Neil Roberts
The SPIR-V spec doesn’t specify a size requirement for these and the
equivalent functions in the GLSL spec have explicit alternatives for
doubles. Refract is a little bit more complicated due to the fact that
the final argument is always supposed to be a scalar 32- or 16- bit
float regardless of the other operands. However in practice it seems
there is a bug in glslang that makes it convert the argument to 64-bit
if you actually try to pass it a 32-bit value while the other
arguments are 64-bit. This adds an optional conversion of the final
argument in order to support any type.

These have been tested against the automatically generated tests of
glsl-4.00/execution/built-in-functions using the ARB_gl_spirv branch
which tests it with quite a large range of combinations.

The issue with glslang has been filed here:
https://github.com/KhronosGroup/glslang/issues/1279

v2: Convert the eta operand of Refract from any size in order to make
it eventually cope with 16-bit floats.
---
 src/compiler/spirv/vtn_glsl450.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/compiler/spirv/vtn_glsl450.c b/src/compiler/spirv/vtn_glsl450.c
index 50783fbfb4d..0cabedf741d 100644
--- a/src/compiler/spirv/vtn_glsl450.c
+++ b/src/compiler/spirv/vtn_glsl450.c
@@ -628,14 +628,14 @@ handle_glsl450_alu(struct vtn_builder *b, enum GLSLstd450 
entrypoint,
case GLSLstd450FaceForward:
   val->ssa->def =
  nir_bcsel(nb, nir_flt(nb, nir_fdot(nb, src[2], src[1]),
-   nir_imm_float(nb, 0.0)),
+   NIR_IMM_FP(nb, 0.0)),
src[0], nir_fneg(nb, src[0]));
   return;
 
case GLSLstd450Reflect:
   /* I - 2 * dot(N, I) * N */
   val->ssa->def =
- nir_fsub(nb, src[0], nir_fmul(nb, nir_imm_float(nb, 2.0),
+ nir_fsub(nb, src[0], nir_fmul(nb, NIR_IMM_FP(nb, 2.0),
   nir_fmul(nb, nir_fdot(nb, src[0], src[1]),
src[1])));
   return;
@@ -645,8 +645,22 @@ handle_glsl450_alu(struct vtn_builder *b, enum GLSLstd450 
entrypoint,
   nir_ssa_def *N = src[1];
   nir_ssa_def *eta = src[2];
   nir_ssa_def *n_dot_i = nir_fdot(nb, N, I);
-  nir_ssa_def *one = nir_imm_float(nb, 1.0);
-  nir_ssa_def *zero = nir_imm_float(nb, 0.0);
+  nir_ssa_def *one = NIR_IMM_FP(nb, 1.0);
+  nir_ssa_def *zero = NIR_IMM_FP(nb, 0.0);
+  /* According to the SPIR-V and GLSL specs, eta is always a float
+   * regardless of the type of the other operands. However in practice it
+   * seems that if you try to pass it a float then glslang will just
+   * promote it to a double and generate invalid SPIR-V. In order to
+   * support a hypothetical fixed version of glslang we’ll promote eta to
+   * double if the other operands are double also.
+   */
+  if (I->bit_size != eta->bit_size) {
+ nir_op conversion_op =
+nir_type_conversion_op(nir_type_float | eta->bit_size,
+   nir_type_float | I->bit_size,
+   nir_rounding_mode_undef);
+ eta = nir_build_alu(nb, conversion_op, eta, NULL, NULL, NULL);
+  }
   /* k = 1.0 - eta * eta * (1.0 - dot(N, I) * dot(N, I)) */
   nir_ssa_def *k =
  nir_fsub(nb, one, nir_fmul(nb, eta, nir_fmul(nb, eta,
-- 
2.14.3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev