[Mesa-dev] [PATCH 2/6] spirv/radv: add AMD_gcn_shader capability, remove current extensions

[Daniel Schürmann]

On 14.03.2018 16:03, Alejandro Piñeiro wrote:
> On 14/03/18 15:55, Daniel Schürmann wrote:
>> Not sure, if I'm asked here :)
>> As AMD_gcn_shader seems to be the only extension without new capability,
>> I am fine with just handling it as if.
> Well, I was exactly asking this, if everybody involved is fine with
> this. Bonus points to get a review to this patch.
>
>> Additionally, we might want to rename it to gcn_shader to be consistent
>> (or add the vendor names to all capabilities).
> Makes sense.
>
>> Do you want to introduce one field per capability or have some
>> capabilities merged (like now)?
> Which capabilities are merged?
storage_16bit: SpvCapabilityStorageUniformBufferBlock16, 
SpvCapabilityStorageUniform16, SpvCapabilityStoragePushConstant16, 
SpvCapabilityStorageInputOutput16
variable_pointers: SpvCapabilityVariablePointersStorageBuffer, 
SpvCapabilityVariablePointers
subgroup_arithmetic: SpvCapabilityGroupNonUniformArithmetic, 
SpvCapabilityGroupNonUniformClustered
subgroup_shuffle: SpvCapabilityGroupNonUniformShuffle, 
SpvCapabilityGroupNonUniformShuffleRelative
tessellation: SpvCapabilityTessellation, SpvCapabilityTessellationPointSize
>>
>> On 11.03.2018 16:25, Alejandro Piñeiro wrote:
>>> FWIW, this is the patch that Im more interested to get a review. It is
>>> also the one that probably would need some discussion. Fortunately this
>>> one can be reviewed independently of the rest of the patches, so the
>>> others can wait a little. Getting this into would make the rebase of
>>> this series more easy.
>>>
>>> So: ping (please)
>>>
>>>
>>> On 08/03/18 16:00, Alejandro Piñeiro wrote:
>>>> So now, during spirv_to_nir, it uses the capability instead of the
>>>> extension. Note that we are really doing here is treating
>>>> SPV_AMD_gcn_shader as other supported extensions. SPV_AMD_gcn_shader
>>>> is not the first SPV extension supported. For example, the capability
>>>> draw_parameters infers if the extension SPV_KHR_shader_draw_parameters
>>>> is supported or not.
>>>>
>>>> This could be seen as counter-intuitive, and that it would be easier
>>>> to define which extensions are supported, and based our checks on
>>>> that, but we need to take into account that some capabilities are
>>>> optional from core, and others came from new extensions.
>>>>
>>>> Also this commit would make the implementation of ARB_spirv_extensions
>>>> easier.
>>>> ---
>>>>
>>>> Note that I'm aware that this can be somewhat confusing at first. But
>>>> most of the SPV extensions defines a new capability, so it makes sense
>>>> to add one, and compute the other based on that. As I mention on a
>>>> different patch on this series, it was easier to compute extensions
>>>> from capabilities, instead of the other way around, because core
>>>> SPIR-V defines optional capabilities without the need of an extension.
>>>>
>>>> Having said so, I have read the SPV_AMD_gcn_shader, and it doesn't
>>>> define a new capability (the first one I see that doesn't do that), so
>>>> I'm somewhat forcing that here.
>>>>
>>>>
>>>>    src/amd/vulkan/radv_shader.c      | 2 --
>>>>    src/compiler/shader_info.h        | 4 ----
>>>>    src/compiler/spirv/nir_spirv.h    | 1 -
>>>>    src/compiler/spirv/spirv_to_nir.c | 2 +-
>>>>    4 files changed, 1 insertion(+), 8 deletions(-)
>>>>
>>>> diff --git a/src/amd/vulkan/radv_shader.c
>>>> b/src/amd/vulkan/radv_shader.c
>>>> index 85672e600d7..46017290654 100644
>>>> --- a/src/amd/vulkan/radv_shader.c
>>>> +++ b/src/amd/vulkan/radv_shader.c
>>>> @@ -214,8 +214,6 @@ radv_shader_compile_to_nir(struct radv_device
>>>> *device,
>>>>                    .multiview = true,
>>>>                    .subgroup_basic = true,
>>>>                    .variable_pointers = true,
>>>> -            },
>>>> -            .exts = {
>>>>                    .AMD_gcn_shader = true,
>>>>                },
>>>>            };
>>>> diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
>>>> index b1e200070f7..502b7901370 100644
>>>> --- a/src/compiler/shader_info.h
>>>> +++ b/src/compiler/shader_info.h
>>>> @@ -51,10 +51,6 @@ struct spirv_supported_capabilities {
>>>>       bool subgroup_quad;
>>>>       bool subgroup_shuffle;
>>>>       bool subgroup_vote;
>>>> -};
>>>> -
>>>> -/* The supported extensions which add extended instructions */
>>>> -struct spirv_supported_extensions {
>>>>       bool AMD_gcn_shader;
>>>>    };
>>>>    diff --git a/src/compiler/spirv/nir_spirv.h
>>>> b/src/compiler/spirv/nir_spirv.h
>>>> index 87d4120c380..d2766abb7f9 100644
>>>> --- a/src/compiler/spirv/nir_spirv.h
>>>> +++ b/src/compiler/spirv/nir_spirv.h
>>>> @@ -60,7 +60,6 @@ struct spirv_to_nir_options {
>>>>       bool lower_workgroup_access_to_offsets;
>>>>         struct spirv_supported_capabilities caps;
>>>> -   struct spirv_supported_extensions exts;
>>>>         struct {
>>>>          void (*func)(void *private_data,
>>>> diff --git a/src/compiler/spirv/spirv_to_nir.c
>>>> b/src/compiler/spirv/spirv_to_nir.c
>>>> index 66b87c049bb..6aa4a4d6b6f 100644
>>>> --- a/src/compiler/spirv/spirv_to_nir.c
>>>> +++ b/src/compiler/spirv/spirv_to_nir.c
>>>> @@ -374,7 +374,7 @@ vtn_handle_extension(struct vtn_builder *b,
>>>> SpvOp opcode,
>>>>          if (strcmp((const char *)&w[2], "GLSL.std.450") == 0) {
>>>>             val->ext_handler = vtn_handle_glsl450_instruction;
>>>>          } else if ((strcmp((const char *)&w[2],
>>>> "SPV_AMD_gcn_shader") == 0)
>>>> -                && (b->options && b->options->exts.AMD_gcn_shader)) {
>>>> +                && (b->options && b->options->caps.AMD_gcn_shader)) {
>>>>             val->ext_handler = vtn_handle_amd_gcn_shader_instruction;
>>>>          } else {
>>>>             vtn_fail("Unsupported extension");
>>