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
> + 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;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
