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

[Daniel Schürmann]

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.

Additionally, we might want to rename it to gcn_shader to be consistent
(or add the vendor names to all capabilities).

Do you want to introduce one field per capability or have some 
capabilities merged (like now)?


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