Re: [Mesa-dev] [PATCH v7 1/2] spirv: add/hookup SpvCapabilityStencilExportEXT

2018-05-11 Thread Jason Ekstrand 
On Fri, May 11, 2018 at 3:06 PM, Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:

> From: Gustavo Lima Chaves 
>
> v2:
> An attempt to support SpvExecutionModeStencilRefReplacingEXT's behavior
> also follows, with the interpretation to said mode being we prevent
> writes to the built-in FragStencilRefEXT variable when the execution
> mode isn't set.
>
> v3:
> A more cautious reading of 1db44252d01bf7539452ccc2b5210c74b8dcd573 led
> me to a missing change that would stop (what I later discovered were)
> GPU hangs on the CTS test written to exercise this.
>
> v4:
> Turn FragStencilRefEXT decoration usage without StencilRefReplacingEXT
> mode into a warning, instead of trying to make the variable read-only.
> If we are to follow the originating extension on GL, the built-in
> variable in question should never be readable anyway.
>
> v5/v6: rebases.
>
> v7:
> Fix check for gen9 lost in rebase. (Ilia)
> Reduce the scope of the bool used to track whether
> SpvExecutionModeStencilRefReplacingEXT was used. Was in shader_info,
> moved to vtn_builder. (Jason)
> ---
>
> Moved the history and some of the code from the other patch. Keeping
> the patch in Gustavo name since the only real change I made was
> trivial (moving a boolean between structs).
>
>  src/compiler/shader_info.h |  1 +
>  src/compiler/spirv/spirv_to_nir.c  |  8 
>  src/compiler/spirv/vtn_private.h   |  1 +
>  src/compiler/spirv/vtn_variables.c | 10 ++
>  4 files changed, 20 insertions(+)
>
> diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
> index afc53a88405..81f844d36ae 100644
> --- a/src/compiler/shader_info.h
> +++ b/src/compiler/shader_info.h
> @@ -56,6 +56,7 @@ struct spirv_supported_capabilities {
> bool trinary_minmax;
> bool descriptor_array_dynamic_indexing;
> bool runtime_descriptor_array;
> +   bool stencil_export;
>  };
>
>  typedef struct shader_info {
> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> index 78437428aa7..bc8e77c35c3 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -3396,6 +3396,10 @@ vtn_handle_preamble_instruction(struct vtn_builder
> *b, SpvOp opcode,
>   spv_check_supported(runtime_descriptor_array, cap);
>   break;
>
> +  case SpvCapabilityStencilExportEXT:
> + spv_check_supported(stencil_export, cap);
> + break;
> +
>default:
>   vtn_fail("Unhandled capability");
>}
> @@ -3573,6 +3577,10 @@ vtn_handle_execution_mode(struct vtn_builder *b,
> struct vtn_value *entry_point,
> case SpvExecutionModeContractionOff:
>break; /* OpenCL */
>
> +   case SpvExecutionModeStencilRefReplacingEXT:
> +  b->outputs_stencil_ref = true;
> +  break;
> +
> default:
>vtn_fail("Unhandled execution mode");
> }
> diff --git a/src/compiler/spirv/vtn_private.h b/src/compiler/spirv/vtn_
> private.h
> index b501bbf9b4a..75f78363b46 100644
> --- a/src/compiler/spirv/vtn_private.h
> +++ b/src/compiler/spirv/vtn_private.h
> @@ -574,6 +574,7 @@ struct vtn_builder {
> struct vtn_value *entry_point;
> bool origin_upper_left;
> bool pixel_center_integer;
> +   bool outputs_stencil_ref;
>
> struct vtn_function *func;
> struct exec_list functions;
> diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_
> variables.c
> index fd8ab7f247a..2bc8b5e9003 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1354,6 +1354,10 @@ vtn_get_builtin_location(struct vtn_builder *b,
>*location = SYSTEM_VALUE_SUBGROUP_LT_MASK,
>set_mode_system_value(b, mode);
>break;
> +   case SpvBuiltInFragStencilRefEXT:
> +  *location = FRAG_RESULT_STENCIL;
> +  vtn_assert(*mode == nir_var_shader_out);
> +  break;
> default:
>vtn_fail("unsupported builtin");
> }
> @@ -1425,6 +1429,12 @@ apply_var_decoration(struct vtn_builder *b,
> nir_variable *nir_var,
>case SpvBuiltInSamplePosition:
>   nir_var->data.origin_upper_left = b->origin_upper_left;
>   break;
> +  case SpvBuiltInFragStencilRefEXT:
> + if (!b->outputs_stencil_ref) {
> +vtn_warn("The StencilRefReplacingEXT mode should be declared
> when"
> + " the decoration FragStencilRefEXT is used on a
> variable");
> + }
> + break;
>

I like this check but, unfortunately, I'm not sure it's actually viable.
The problem is that execution modes (which is what this is checking) are
applied to a shader entrypoint.  Since you can have multiple entrypoints
per module and this variable may not be used for the primary entrypoint,
I'm not sure it makes sense.  Maybe best to just drop it and hope they do
the right thing.


>default:
>   break;
>}
> --
> 2.17.0
>
> ___
> mesa-dev

[Mesa-dev] [PATCH v7 1/2] spirv: add/hookup SpvCapabilityStencilExportEXT

2018-05-11 Thread Caio Marcelo de Oliveira Filho 
From: Gustavo Lima Chaves 

v2:
An attempt to support SpvExecutionModeStencilRefReplacingEXT's behavior
also follows, with the interpretation to said mode being we prevent
writes to the built-in FragStencilRefEXT variable when the execution
mode isn't set.

v3:
A more cautious reading of 1db44252d01bf7539452ccc2b5210c74b8dcd573 led
me to a missing change that would stop (what I later discovered were)
GPU hangs on the CTS test written to exercise this.

v4:
Turn FragStencilRefEXT decoration usage without StencilRefReplacingEXT
mode into a warning, instead of trying to make the variable read-only.
If we are to follow the originating extension on GL, the built-in
variable in question should never be readable anyway.

v5/v6: rebases.

v7:
Fix check for gen9 lost in rebase. (Ilia)
Reduce the scope of the bool used to track whether
SpvExecutionModeStencilRefReplacingEXT was used. Was in shader_info,
moved to vtn_builder. (Jason)
---

Moved the history and some of the code from the other patch. Keeping
the patch in Gustavo name since the only real change I made was
trivial (moving a boolean between structs).

 src/compiler/shader_info.h |  1 +
 src/compiler/spirv/spirv_to_nir.c  |  8 
 src/compiler/spirv/vtn_private.h   |  1 +
 src/compiler/spirv/vtn_variables.c | 10 ++
 4 files changed, 20 insertions(+)

diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
index afc53a88405..81f844d36ae 100644
--- a/src/compiler/shader_info.h
+++ b/src/compiler/shader_info.h
@@ -56,6 +56,7 @@ struct spirv_supported_capabilities {
bool trinary_minmax;
bool descriptor_array_dynamic_indexing;
bool runtime_descriptor_array;
+   bool stencil_export;
 };
 
 typedef struct shader_info {
diff --git a/src/compiler/spirv/spirv_to_nir.c 
b/src/compiler/spirv/spirv_to_nir.c
index 78437428aa7..bc8e77c35c3 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -3396,6 +3396,10 @@ vtn_handle_preamble_instruction(struct vtn_builder *b, 
SpvOp opcode,
  spv_check_supported(runtime_descriptor_array, cap);
  break;
 
+  case SpvCapabilityStencilExportEXT:
+ spv_check_supported(stencil_export, cap);
+ break;
+
   default:
  vtn_fail("Unhandled capability");
   }
@@ -3573,6 +3577,10 @@ vtn_handle_execution_mode(struct vtn_builder *b, struct 
vtn_value *entry_point,
case SpvExecutionModeContractionOff:
   break; /* OpenCL */
 
+   case SpvExecutionModeStencilRefReplacingEXT:
+  b->outputs_stencil_ref = true;
+  break;
+
default:
   vtn_fail("Unhandled execution mode");
}
diff --git a/src/compiler/spirv/vtn_private.h b/src/compiler/spirv/vtn_private.h
index b501bbf9b4a..75f78363b46 100644
--- a/src/compiler/spirv/vtn_private.h
+++ b/src/compiler/spirv/vtn_private.h
@@ -574,6 +574,7 @@ struct vtn_builder {
struct vtn_value *entry_point;
bool origin_upper_left;
bool pixel_center_integer;
+   bool outputs_stencil_ref;
 
struct vtn_function *func;
struct exec_list functions;
diff --git a/src/compiler/spirv/vtn_variables.c 
b/src/compiler/spirv/vtn_variables.c
index fd8ab7f247a..2bc8b5e9003 100644
--- a/src/compiler/spirv/vtn_variables.c
+++ b/src/compiler/spirv/vtn_variables.c
@@ -1354,6 +1354,10 @@ vtn_get_builtin_location(struct vtn_builder *b,
   *location = SYSTEM_VALUE_SUBGROUP_LT_MASK,
   set_mode_system_value(b, mode);
   break;
+   case SpvBuiltInFragStencilRefEXT:
+  *location = FRAG_RESULT_STENCIL;
+  vtn_assert(*mode == nir_var_shader_out);
+  break;
default:
   vtn_fail("unsupported builtin");
}
@@ -1425,6 +1429,12 @@ apply_var_decoration(struct vtn_builder *b, nir_variable 
*nir_var,
   case SpvBuiltInSamplePosition:
  nir_var->data.origin_upper_left = b->origin_upper_left;
  break;
+  case SpvBuiltInFragStencilRefEXT:
+ if (!b->outputs_stencil_ref) {
+vtn_warn("The StencilRefReplacingEXT mode should be declared when"
+ " the decoration FragStencilRefEXT is used on a 
variable");
+ }
+ break;
   default:
  break;
   }
-- 
2.17.0

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