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