Re: [Mesa-dev] [PATCH v3] anv: enable VK_EXT_shader_stencil_export

[Jason Ekstrand]

On Fri, Feb 23, 2018 at 1:40 AM, Iago Toral  wrote:

> On Thu, 2018-02-22 at 11:22 -0800, Gustavo Lima Chaves wrote:
> > 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 exercize this.
> > ---
> >  src/compiler/shader_info.h | 2 ++
> >  src/compiler/spirv/spirv_to_nir.c  | 4 
> >  src/compiler/spirv/vtn_variables.c | 4 
> >  src/intel/vulkan/anv_extensions.py | 2 ++
> >  src/intel/vulkan/anv_pipeline.c| 1 +
> >  src/intel/vulkan/genX_pipeline.c   | 1 +
> >  6 files changed, 14 insertions(+)
> >
> > diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
> > index 6de707f672..f99cbc27a7 100644
> > --- a/src/compiler/shader_info.h
> > +++ b/src/compiler/shader_info.h
> > @@ -162,6 +162,8 @@ typedef struct shader_info {
> >
> >   bool pixel_center_integer;
> >
> > + bool outputs_stencil;
> > +
> >   /** gl_FragDepth layout for ARB_conservative_depth. */
> >   enum gl_frag_depth_layout depth_layout;
> >} fs;
> > diff --git a/src/compiler/spirv/spirv_to_nir.c
> > b/src/compiler/spirv/spirv_to_nir.c
> > index e00dcafa12..dcb8b31967 100644
> > --- a/src/compiler/spirv/spirv_to_nir.c
> > +++ b/src/compiler/spirv/spirv_to_nir.c
> > @@ -3395,6 +3395,10 @@ vtn_handle_execution_mode(struct vtn_builder
> > *b, struct vtn_value *entry_point,
> > case SpvExecutionModeContractionOff:
> >break; /* OpenCL */
> >
> > +   case SpvExecutionModeStencilRefReplacingEXT:
> > +  b->shader->info.fs.outputs_stencil = true;
> > +  break;
> > +
> > default:
> >vtn_fail("Unhandled execution mode");
> > }
> > diff --git a/src/compiler/spirv/vtn_variables.c
> > b/src/compiler/spirv/vtn_variables.c
> > index 36976798e9..42f915d434 100644
> > --- a/src/compiler/spirv/vtn_variables.c
> > +++ b/src/compiler/spirv/vtn_variables.c
> > @@ -1373,6 +1373,10 @@ apply_var_decoration(struct vtn_builder *b,
> > nir_variable *nir_var,
> >case SpvBuiltInFragCoord:
> >   nir_var->data.pixel_center_integer = b-
> > >pixel_center_integer;
> >   break;
> > +  case SpvBuiltInFragStencilRefEXT:
> > + if (!b->shader->info.fs.outputs_stencil)
> > + nir_var->data.read_only = true;
> > + break;
>
> From the SPIR-V spec:
>
> FragStencilRefEXT must only decorate output variable whose type is
> an arbitrary-sized integer type scalar.
>
> If it is an output variable, I don't think we should make it read-only
> in any case.
>

Thanks for doing the spec archeology.  I suspected that was the case.


> More over, from ARB_shader_stencil_export (which is the base GL
> extension this is trying to replicate):
>
> "1) Should gl_FragStencilRefARB be initialized to the current stencil
> reference value on input to the fragment shader?
>
> RESOLVED: No. gl_FragStencilRefARB is write-only. If the current
> stencil reference value is required in a shader, the application should
> place it in a uniform."
>
> In other words, this is an output-only variable and it is not expected
> to be read. If we see a SpvBuiltInFragStencilRefEXT decoration then my
> understanding is that we should also have
> SpvExecutionModeStencilRefReplacingEXT, so if we didn't have that I
> think we have incorrect SPIR-V, and in that case, instead of marking
> the variable as read-only, we should maybe just drop a warning instead.
>

Thanks for checking.  I've filed an issue against the Vulkan spec for this:

https://gitlab.khronos.org/vulkan/vulkan/issues/1173

For now, let's proceed under the assumption that the Vulkan extension
follows the GL extension and does not require gl_FragStencilRefARB to be
initialized with the API stencil reference value.

--Jason


> >default:
> >   break;
> >}
> > diff --git a/src/intel/vulkan/anv_extensions.py
> > b/src/intel/vulkan/anv_extensions.py
> > index 581921e62a..cd90c6ae52 100644
> > --- a/src/intel/vulkan/anv_extensions.py
> > +++ b/src/intel/vulkan/anv_extensions.py
> > @@ -86,6 +86,8 @@ EXTENSIONS = [
> >  Extension('VK_KHX_multiview', 1, True),
> >  Extension('VK_EXT_debug_report',  8, True),
> >  Extension('VK_EXT_external_memory_dma_buf',   1, True),
> > +Extension('VK_EXT_shader_stencil_export', 1,
> > +  'device->info.gen >= 9'),
> >  ]
> >
> >  class VkVersion:
> > diff --git a/src/intel/vulkan/anv_pipeline.c
> > b/src/intel/vulkan/anv_pipeline.c
> > index e16a7a1994..ed63fa42cd 100644
> > --- a/src/intel/vulkan/anv_pipeline.c
> > +++ 

Re: [Mesa-dev] [PATCH v3] anv: enable VK_EXT_shader_stencil_export

[Iago Toral]

On Thu, 2018-02-22 at 11:22 -0800, Gustavo Lima Chaves wrote:
> 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 exercize this.
> ---
>  src/compiler/shader_info.h | 2 ++
>  src/compiler/spirv/spirv_to_nir.c  | 4 
>  src/compiler/spirv/vtn_variables.c | 4 
>  src/intel/vulkan/anv_extensions.py | 2 ++
>  src/intel/vulkan/anv_pipeline.c| 1 +
>  src/intel/vulkan/genX_pipeline.c   | 1 +
>  6 files changed, 14 insertions(+)
> 
> diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
> index 6de707f672..f99cbc27a7 100644
> --- a/src/compiler/shader_info.h
> +++ b/src/compiler/shader_info.h
> @@ -162,6 +162,8 @@ typedef struct shader_info {
>  
>   bool pixel_center_integer;
>  
> + bool outputs_stencil;
> +
>   /** gl_FragDepth layout for ARB_conservative_depth. */
>   enum gl_frag_depth_layout depth_layout;
>} fs;
> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> index e00dcafa12..dcb8b31967 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -3395,6 +3395,10 @@ vtn_handle_execution_mode(struct vtn_builder
> *b, struct vtn_value *entry_point,
> case SpvExecutionModeContractionOff:
>break; /* OpenCL */
>  
> +   case SpvExecutionModeStencilRefReplacingEXT:
> +  b->shader->info.fs.outputs_stencil = true;
> +  break;
> +
> default:
>vtn_fail("Unhandled execution mode");
> }
> diff --git a/src/compiler/spirv/vtn_variables.c
> b/src/compiler/spirv/vtn_variables.c
> index 36976798e9..42f915d434 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1373,6 +1373,10 @@ apply_var_decoration(struct vtn_builder *b,
> nir_variable *nir_var,
>case SpvBuiltInFragCoord:
>   nir_var->data.pixel_center_integer = b-
> >pixel_center_integer;
>   break;
> +  case SpvBuiltInFragStencilRefEXT:
> + if (!b->shader->info.fs.outputs_stencil)
> + nir_var->data.read_only = true;
> + break;

From the SPIR-V spec:

FragStencilRefEXT must only decorate output variable whose type is
an arbitrary-sized integer type scalar.

If it is an output variable, I don't think we should make it read-only
in any case.

More over, from ARB_shader_stencil_export (which is the base GL
extension this is trying to replicate):

"1) Should gl_FragStencilRefARB be initialized to the current stencil
reference value on input to the fragment shader?

RESOLVED: No. gl_FragStencilRefARB is write-only. If the current
stencil reference value is required in a shader, the application should
place it in a uniform."

In other words, this is an output-only variable and it is not expected
to be read. If we see a SpvBuiltInFragStencilRefEXT decoration then my
understanding is that we should also have
SpvExecutionModeStencilRefReplacingEXT, so if we didn't have that I
think we have incorrect SPIR-V, and in that case, instead of marking
the variable as read-only, we should maybe just drop a warning instead.


>default:
>   break;
>}
> diff --git a/src/intel/vulkan/anv_extensions.py
> b/src/intel/vulkan/anv_extensions.py
> index 581921e62a..cd90c6ae52 100644
> --- a/src/intel/vulkan/anv_extensions.py
> +++ b/src/intel/vulkan/anv_extensions.py
> @@ -86,6 +86,8 @@ EXTENSIONS = [
>  Extension('VK_KHX_multiview', 1, True),
>  Extension('VK_EXT_debug_report',  8, True),
>  Extension('VK_EXT_external_memory_dma_buf',   1, True),
> +Extension('VK_EXT_shader_stencil_export', 1,
> +  'device->info.gen >= 9'),
>  ]
>  
>  class VkVersion:
> diff --git a/src/intel/vulkan/anv_pipeline.c
> b/src/intel/vulkan/anv_pipeline.c
> index e16a7a1994..ed63fa42cd 100644
> --- a/src/intel/vulkan/anv_pipeline.c
> +++ b/src/intel/vulkan/anv_pipeline.c
> @@ -143,6 +143,7 @@ anv_shader_compile_to_nir(struct anv_pipeline
> *pipeline,
>   .multiview = true,
>   .variable_pointers = true,
>   .storage_16bit = device->instance->physicalDevice.info.gen
> >= 8,
> + .stencil_export = device->instance->physicalDevice.info.gen 
> >= 9,
>},
> };
>  
> diff --git a/src/intel/vulkan/genX_pipeline.c
> b/src/intel/vulkan/genX_pipeline.c
> index 89cbe293b8..683a4607e6 100644
> --- a/src/intel/vulkan/genX_pipeline.c
> +++ b/src/intel/vulkan/genX_pipeline.c
> @@ -1600,6 +1600,7 @@ emit_3dstate_ps_extra(struct anv_pipeline
> *pipeline,
>   ps.PixelShaderHasUAV = true;
>  
>  #if GEN_GEN >= 9
> 

[Mesa-dev] [PATCH v3] anv: enable VK_EXT_shader_stencil_export

[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 exercize this.
---
 src/compiler/shader_info.h | 2 ++
 src/compiler/spirv/spirv_to_nir.c  | 4 
 src/compiler/spirv/vtn_variables.c | 4 
 src/intel/vulkan/anv_extensions.py | 2 ++
 src/intel/vulkan/anv_pipeline.c| 1 +
 src/intel/vulkan/genX_pipeline.c   | 1 +
 6 files changed, 14 insertions(+)

diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
index 6de707f672..f99cbc27a7 100644
--- a/src/compiler/shader_info.h
+++ b/src/compiler/shader_info.h
@@ -162,6 +162,8 @@ typedef struct shader_info {
 
  bool pixel_center_integer;
 
+ bool outputs_stencil;
+
  /** gl_FragDepth layout for ARB_conservative_depth. */
  enum gl_frag_depth_layout depth_layout;
   } fs;
diff --git a/src/compiler/spirv/spirv_to_nir.c 
b/src/compiler/spirv/spirv_to_nir.c
index e00dcafa12..dcb8b31967 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -3395,6 +3395,10 @@ vtn_handle_execution_mode(struct vtn_builder *b, struct 
vtn_value *entry_point,
case SpvExecutionModeContractionOff:
   break; /* OpenCL */
 
+   case SpvExecutionModeStencilRefReplacingEXT:
+  b->shader->info.fs.outputs_stencil = true;
+  break;
+
default:
   vtn_fail("Unhandled execution mode");
}
diff --git a/src/compiler/spirv/vtn_variables.c 
b/src/compiler/spirv/vtn_variables.c
index 36976798e9..42f915d434 100644
--- a/src/compiler/spirv/vtn_variables.c
+++ b/src/compiler/spirv/vtn_variables.c
@@ -1373,6 +1373,10 @@ apply_var_decoration(struct vtn_builder *b, nir_variable 
*nir_var,
   case SpvBuiltInFragCoord:
  nir_var->data.pixel_center_integer = b->pixel_center_integer;
  break;
+  case SpvBuiltInFragStencilRefEXT:
+ if (!b->shader->info.fs.outputs_stencil)
+ nir_var->data.read_only = true;
+ break;
   default:
  break;
   }
diff --git a/src/intel/vulkan/anv_extensions.py 
b/src/intel/vulkan/anv_extensions.py
index 581921e62a..cd90c6ae52 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -86,6 +86,8 @@ EXTENSIONS = [
 Extension('VK_KHX_multiview', 1, True),
 Extension('VK_EXT_debug_report',  8, True),
 Extension('VK_EXT_external_memory_dma_buf',   1, True),
+Extension('VK_EXT_shader_stencil_export', 1,
+  'device->info.gen >= 9'),
 ]
 
 class VkVersion:
diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c
index e16a7a1994..ed63fa42cd 100644
--- a/src/intel/vulkan/anv_pipeline.c
+++ b/src/intel/vulkan/anv_pipeline.c
@@ -143,6 +143,7 @@ anv_shader_compile_to_nir(struct anv_pipeline *pipeline,
  .multiview = true,
  .variable_pointers = true,
  .storage_16bit = device->instance->physicalDevice.info.gen >= 8,
+ .stencil_export = device->instance->physicalDevice.info.gen >= 9,
   },
};
 
diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index 89cbe293b8..683a4607e6 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -1600,6 +1600,7 @@ emit_3dstate_ps_extra(struct anv_pipeline *pipeline,
  ps.PixelShaderHasUAV = true;
 
 #if GEN_GEN >= 9
+  ps.PixelShaderComputesStencil = wm_prog_data->computed_stencil;
   ps.PixelShaderPullsBary= wm_prog_data->pulls_bary;
   ps.InputCoverageMaskState  = wm_prog_data->uses_sample_mask ?
ICMS_INNER_CONSERVATIVE : ICMS_NONE;
-- 
2.14.3

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