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

[Alejandro Piñeiro]

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?

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