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

[Alejandro Piñeiro]

On 14/03/18 16:08, Daniel Schürmann wrote:
>
> 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

Oh true. Thanks for the detailed list. So now replying to your question:
I think that it would be better to keep capabilities merged. Mostly
because it is working right now, and I don't see any big advantage to
start to split it, unless we want start to fine-grain spirv_to_nir
support for each capability defined at each extension, and that seems a
little overkill.

>>>
>>> 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");
>>>
>
>