Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Emil Velikov]

On 13 September 2017 at 23:31, Ian Romanick  wrote:
> On 08/29/2017 10:19 AM, Emil Velikov wrote:
>> On 29 August 2017 at 18:10, Matt Turner  wrote:
>>> On Tue, Aug 29, 2017 at 3:35 AM, Emil Velikov  
>>> wrote:
 On 29 August 2017 at 11:11, Eric Engestrom  
 wrote:
> On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote:
>> Avoids Clang's warning about the current code:
>>
>>warning: suggest braces around initialization of subobject
>
> I'm not sure about this patch [1], but everything else in this series is:
 IIRC "{}" is the least likely way to avoid the warnings, across
 GCC/Clang versions.
 It's not part of the C spec, but since it works - not sure how much to 
 bother.
>>>
>>> I'm surprised to hear you say that. I've tested with gcc-4.9.4, 6.4.0,
>>> 7.1.0; and clang-4.0, and none of them warn about {}
>>>
>> Eric corrected me on IRC, but I forgot to mention here - "s/least/most/"
>>
>>> I think when we went through this before (in NIR) we couldn't use {}
>>> because it needs to compile with MSVC, and MSVC doesn't allow that in
>>> C.
>>>
>> Precisely. The "most" portable solution that I know of is memset().
>
> Except memset generates different code.  That will clear the padding
> area between fields, and x = {} (and variations there of) do not.
>
 Would be great to have the issue reported (and fixed) in Clang though.
 AFAICT both {0} and {0,} are valid, if memory serves me right.
>>>
>>> I don't really understand the nuances of {0}, {0,}, {{0}}, etc, and I
>>> get the sense that that's the case for nearly everyone else as well.
>>>
>> AFAICT {0} and {0,} should just work everywhere, modulo bugs.
>
> And I believe most of these generate warnings if you enable other
> warning options (see also this patch series
> https://patchwork.freedesktop.org/patch/68141/).
>
Even with maximum amount of warning flags, what gets flagged depends
on compiler + version.
Aka the "module bugs" mentioned above - feel free to skim through the
bug reports linked.

That said - yes, handling these is rather annoying :-\

>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
>> [2] https://bugs.llvm.org/show_bug.cgi?id=21689
>>

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Ian Romanick]

On 08/29/2017 10:19 AM, Emil Velikov wrote:
> On 29 August 2017 at 18:10, Matt Turner  wrote:
>> On Tue, Aug 29, 2017 at 3:35 AM, Emil Velikov  
>> wrote:
>>> On 29 August 2017 at 11:11, Eric Engestrom  
>>> wrote:
 On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote:
> Avoids Clang's warning about the current code:
>
>warning: suggest braces around initialization of subobject

 I'm not sure about this patch [1], but everything else in this series is:
>>> IIRC "{}" is the least likely way to avoid the warnings, across
>>> GCC/Clang versions.
>>> It's not part of the C spec, but since it works - not sure how much to 
>>> bother.
>>
>> I'm surprised to hear you say that. I've tested with gcc-4.9.4, 6.4.0,
>> 7.1.0; and clang-4.0, and none of them warn about {}
>>
> Eric corrected me on IRC, but I forgot to mention here - "s/least/most/"
> 
>> I think when we went through this before (in NIR) we couldn't use {}
>> because it needs to compile with MSVC, and MSVC doesn't allow that in
>> C.
>>
> Precisely. The "most" portable solution that I know of is memset().

Except memset generates different code.  That will clear the padding
area between fields, and x = {} (and variations there of) do not.

>>> Would be great to have the issue reported (and fixed) in Clang though.
>>> AFAICT both {0} and {0,} are valid, if memory serves me right.
>>
>> I don't really understand the nuances of {0}, {0,}, {{0}}, etc, and I
>> get the sense that that's the case for nearly everyone else as well.
>>
> AFAICT {0} and {0,} should just work everywhere, modulo bugs.

And I believe most of these generate warnings if you enable other
warning options (see also this patch series
https://patchwork.freedesktop.org/patch/68141/).

> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> [2] https://bugs.llvm.org/show_bug.cgi?id=21689
> 
>> The most prominent bug I see in all of this is that {} isn't part of
>> standard C (it is part of standard C++!)
> Agreed. Not sure how easy it will be to convince the committee.
> 
> -Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Eric Engestrom]

On 29 August 2017 18:11:45 BST, Matt Turner  wrote:
> On Tue, Aug 29, 2017 at 3:11 AM, Eric Engestrom
>  wrote:
> > On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote:
> >> Avoids Clang's warning about the current code:
> >>
> >>warning: suggest braces around initialization of subobject
> >
> > I'm not sure about this patch [1], but everything else in this
> series is:
> > Reviewed-by: Eric Engestrom 
> >
> > The "mark `UNUSED` because they are unused in some generations"
> patches
> > might be better to mark as `MAYBE_UNUSED`; they both resolve to the
> same
> > thing for compilers, but the latter is gives more information to
> humans
> > (ie. "this isn't always unused").
> >
> > [1]
> > Can you test this with GCC 4.x and 7.x? If memory serves, this init
> format
> > caused warnings on either old GCC or new GCC.
> > I think `= {{0}};` was the only format that both compilers were
> happy with.
> 
> Tested 4.9.4 and 7.1.0. No warnings from either of them about these
> initializers :)

Given this and Emil's comment, this patch is r-b me with the rest of the series 
:)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Emil Velikov]

On 29 August 2017 at 18:10, Matt Turner  wrote:
> On Tue, Aug 29, 2017 at 3:35 AM, Emil Velikov  
> wrote:
>> On 29 August 2017 at 11:11, Eric Engestrom  wrote:
>>> On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote:
 Avoids Clang's warning about the current code:

warning: suggest braces around initialization of subobject
>>>
>>> I'm not sure about this patch [1], but everything else in this series is:
>> IIRC "{}" is the least likely way to avoid the warnings, across
>> GCC/Clang versions.
>> It's not part of the C spec, but since it works - not sure how much to 
>> bother.
>
> I'm surprised to hear you say that. I've tested with gcc-4.9.4, 6.4.0,
> 7.1.0; and clang-4.0, and none of them warn about {}
>
Eric corrected me on IRC, but I forgot to mention here - "s/least/most/"

> I think when we went through this before (in NIR) we couldn't use {}
> because it needs to compile with MSVC, and MSVC doesn't allow that in
> C.
>
Precisely. The "most" portable solution that I know of is memset().

>> Would be great to have the issue reported (and fixed) in Clang though.
>> AFAICT both {0} and {0,} are valid, if memory serves me right.
>
> I don't really understand the nuances of {0}, {0,}, {{0}}, etc, and I
> get the sense that that's the case for nearly everyone else as well.
>
AFAICT {0} and {0,} should just work everywhere, modulo bugs.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
[2] https://bugs.llvm.org/show_bug.cgi?id=21689

> The most prominent bug I see in all of this is that {} isn't part of
> standard C (it is part of standard C++!)
Agreed. Not sure how easy it will be to convince the committee.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Matt Turner]

On Tue, Aug 29, 2017 at 3:11 AM, Eric Engestrom
 wrote:
> On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote:
>> Avoids Clang's warning about the current code:
>>
>>warning: suggest braces around initialization of subobject
>
> I'm not sure about this patch [1], but everything else in this series is:
> Reviewed-by: Eric Engestrom 
>
> The "mark `UNUSED` because they are unused in some generations" patches
> might be better to mark as `MAYBE_UNUSED`; they both resolve to the same
> thing for compilers, but the latter is gives more information to humans
> (ie. "this isn't always unused").
>
> [1]
> Can you test this with GCC 4.x and 7.x? If memory serves, this init format
> caused warnings on either old GCC or new GCC.
> I think `= {{0}};` was the only format that both compilers were happy with.

Tested 4.9.4 and 7.1.0. No warnings from either of them about these
initializers :)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Matt Turner]

On Tue, Aug 29, 2017 at 3:35 AM, Emil Velikov  wrote:
> On 29 August 2017 at 11:11, Eric Engestrom  wrote:
>> On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote:
>>> Avoids Clang's warning about the current code:
>>>
>>>warning: suggest braces around initialization of subobject
>>
>> I'm not sure about this patch [1], but everything else in this series is:
> IIRC "{}" is the least likely way to avoid the warnings, across
> GCC/Clang versions.
> It's not part of the C spec, but since it works - not sure how much to bother.

I'm surprised to hear you say that. I've tested with gcc-4.9.4, 6.4.0,
7.1.0; and clang-4.0, and none of them warn about {}

I think when we went through this before (in NIR) we couldn't use {}
because it needs to compile with MSVC, and MSVC doesn't allow that in
C.

> Would be great to have the issue reported (and fixed) in Clang though.
> AFAICT both {0} and {0,} are valid, if memory serves me right.

I don't really understand the nuances of {0}, {0,}, {{0}}, etc, and I
get the sense that that's the case for nearly everyone else as well.

The most prominent bug I see in all of this is that {} isn't part of
standard C (it is part of standard C++!)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Emil Velikov]

On 29 August 2017 at 11:11, Eric Engestrom  wrote:
> On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote:
>> Avoids Clang's warning about the current code:
>>
>>warning: suggest braces around initialization of subobject
>
> I'm not sure about this patch [1], but everything else in this series is:
IIRC "{}" is the least likely way to avoid the warnings, across
GCC/Clang versions.
It's not part of the C spec, but since it works - not sure how much to bother.

Would be great to have the issue reported (and fixed) in Clang though.
AFAICT both {0} and {0,} are valid, if memory serves me right.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Eric Engestrom]

On Monday, 2017-08-28 14:57:13 -0700, Matt Turner wrote:
> Avoids Clang's warning about the current code:
> 
>warning: suggest braces around initialization of subobject

I'm not sure about this patch [1], but everything else in this series is:
Reviewed-by: Eric Engestrom 

The "mark `UNUSED` because they are unused in some generations" patches
might be better to mark as `MAYBE_UNUSED`; they both resolve to the same
thing for compilers, but the latter is gives more information to humans
(ie. "this isn't always unused").

[1]
Can you test this with GCC 4.x and 7.x? If memory serves, this init format
caused warnings on either old GCC or new GCC.
I think `= {{0}};` was the only format that both compilers were happy with.
CC'ing Emil who might remember more/better than me.

> ---
>  src/intel/vulkan/anv_formats.c  |  2 +-
>  src/intel/vulkan/anv_pipeline.c | 20 ++--
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/intel/vulkan/anv_formats.c b/src/intel/vulkan/anv_formats.c
> index 9808508523..6ce609ae8d 100644
> --- a/src/intel/vulkan/anv_formats.c
> +++ b/src/intel/vulkan/anv_formats.c
> @@ -739,7 +739,7 @@ VkResult anv_GetPhysicalDeviceImageFormatProperties2KHR(
> *the implementation for use in vkCreateImage, then all members of
> *imageFormatProperties will be filled with zero.
> */
> -  base_props->imageFormatProperties = (VkImageFormatProperties) {0};
> +  base_props->imageFormatProperties = (VkImageFormatProperties) {};
> }
>  
> return result;
> diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c
> index 279d76561a..8306cd366a 100644
> --- a/src/intel/vulkan/anv_pipeline.c
> +++ b/src/intel/vulkan/anv_pipeline.c
> @@ -510,7 +510,7 @@ anv_pipeline_compile_vs(struct anv_pipeline *pipeline,
> }
>  
> if (bin == NULL) {
> -  struct brw_vs_prog_data prog_data = { 0, };
> +  struct brw_vs_prog_data prog_data = {};
>struct anv_pipeline_binding surface_to_descriptor[256];
>struct anv_pipeline_binding sampler_to_descriptor[256];
>  
> @@ -617,8 +617,8 @@ anv_pipeline_compile_tcs_tes(struct anv_pipeline 
> *pipeline,
>pipeline->device->instance->physicalDevice.compiler;
> struct anv_pipeline_bind_map tcs_map;
> struct anv_pipeline_bind_map tes_map;
> -   struct brw_tcs_prog_key tcs_key = { 0, };
> -   struct brw_tes_prog_key tes_key = { 0, };
> +   struct brw_tcs_prog_key tcs_key = {};
> +   struct brw_tes_prog_key tes_key = {};
> struct anv_shader_bin *tcs_bin = NULL;
> struct anv_shader_bin *tes_bin = NULL;
> unsigned char tcs_sha1[40];
> @@ -642,8 +642,8 @@ anv_pipeline_compile_tcs_tes(struct anv_pipeline 
> *pipeline,
> }
>  
> if (tcs_bin == NULL || tes_bin == NULL) {
> -  struct brw_tcs_prog_data tcs_prog_data = { 0, };
> -  struct brw_tes_prog_data tes_prog_data = { 0, };
> +  struct brw_tcs_prog_data tcs_prog_data = {};
> +  struct brw_tes_prog_data tes_prog_data = {};
>struct anv_pipeline_binding tcs_surface_to_descriptor[256];
>struct anv_pipeline_binding tcs_sampler_to_descriptor[256];
>struct anv_pipeline_binding tes_surface_to_descriptor[256];
> @@ -775,7 +775,7 @@ anv_pipeline_compile_gs(struct anv_pipeline *pipeline,
> }
>  
> if (bin == NULL) {
> -  struct brw_gs_prog_data prog_data = { 0, };
> +  struct brw_gs_prog_data prog_data = {};
>struct anv_pipeline_binding surface_to_descriptor[256];
>struct anv_pipeline_binding sampler_to_descriptor[256];
>  
> @@ -853,7 +853,7 @@ anv_pipeline_compile_fs(struct anv_pipeline *pipeline,
> }
>  
> if (bin == NULL) {
> -  struct brw_wm_prog_data prog_data = { 0, };
> +  struct brw_wm_prog_data prog_data = {};
>struct anv_pipeline_binding surface_to_descriptor[256];
>struct anv_pipeline_binding sampler_to_descriptor[256];
>  
> @@ -976,7 +976,7 @@ anv_pipeline_compile_cs(struct anv_pipeline *pipeline,
> }
>  
> if (bin == NULL) {
> -  struct brw_cs_prog_data prog_data = { 0, };
> +  struct brw_cs_prog_data prog_data = {};
>struct anv_pipeline_binding surface_to_descriptor[256];
>struct anv_pipeline_binding sampler_to_descriptor[256];
>  
> @@ -1277,8 +1277,8 @@ anv_pipeline_init(struct anv_pipeline *pipeline,
>  
> pipeline->active_stages = 0;
>  
> -   const VkPipelineShaderStageCreateInfo *pStages[MESA_SHADER_STAGES] = { 0, 
> };
> -   struct anv_shader_module *modules[MESA_SHADER_STAGES] = { 0, };
> +   const VkPipelineShaderStageCreateInfo *pStages[MESA_SHADER_STAGES] = {};
> +   struct anv_shader_module *modules[MESA_SHADER_STAGES] = {};
> for (uint32_t i = 0; i < pCreateInfo->stageCount; i++) {
>gl_shader_stage stage = ffs(pCreateInfo->pStages[i].stage) - 1;
>pStages[stage] = >pStages[i];
> -- 
> 2.13.5
> 
___
mesa-dev mailing list

[Mesa-dev] [PATCH 11/19] anv: Use GNU C empty brace initializer

[Matt Turner]

Avoids Clang's warning about the current code:

   warning: suggest braces around initialization of subobject
---
 src/intel/vulkan/anv_formats.c  |  2 +-
 src/intel/vulkan/anv_pipeline.c | 20 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/intel/vulkan/anv_formats.c b/src/intel/vulkan/anv_formats.c
index 9808508523..6ce609ae8d 100644
--- a/src/intel/vulkan/anv_formats.c
+++ b/src/intel/vulkan/anv_formats.c
@@ -739,7 +739,7 @@ VkResult anv_GetPhysicalDeviceImageFormatProperties2KHR(
*the implementation for use in vkCreateImage, then all members of
*imageFormatProperties will be filled with zero.
*/
-  base_props->imageFormatProperties = (VkImageFormatProperties) {0};
+  base_props->imageFormatProperties = (VkImageFormatProperties) {};
}
 
return result;
diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c
index 279d76561a..8306cd366a 100644
--- a/src/intel/vulkan/anv_pipeline.c
+++ b/src/intel/vulkan/anv_pipeline.c
@@ -510,7 +510,7 @@ anv_pipeline_compile_vs(struct anv_pipeline *pipeline,
}
 
if (bin == NULL) {
-  struct brw_vs_prog_data prog_data = { 0, };
+  struct brw_vs_prog_data prog_data = {};
   struct anv_pipeline_binding surface_to_descriptor[256];
   struct anv_pipeline_binding sampler_to_descriptor[256];
 
@@ -617,8 +617,8 @@ anv_pipeline_compile_tcs_tes(struct anv_pipeline *pipeline,
   pipeline->device->instance->physicalDevice.compiler;
struct anv_pipeline_bind_map tcs_map;
struct anv_pipeline_bind_map tes_map;
-   struct brw_tcs_prog_key tcs_key = { 0, };
-   struct brw_tes_prog_key tes_key = { 0, };
+   struct brw_tcs_prog_key tcs_key = {};
+   struct brw_tes_prog_key tes_key = {};
struct anv_shader_bin *tcs_bin = NULL;
struct anv_shader_bin *tes_bin = NULL;
unsigned char tcs_sha1[40];
@@ -642,8 +642,8 @@ anv_pipeline_compile_tcs_tes(struct anv_pipeline *pipeline,
}
 
if (tcs_bin == NULL || tes_bin == NULL) {
-  struct brw_tcs_prog_data tcs_prog_data = { 0, };
-  struct brw_tes_prog_data tes_prog_data = { 0, };
+  struct brw_tcs_prog_data tcs_prog_data = {};
+  struct brw_tes_prog_data tes_prog_data = {};
   struct anv_pipeline_binding tcs_surface_to_descriptor[256];
   struct anv_pipeline_binding tcs_sampler_to_descriptor[256];
   struct anv_pipeline_binding tes_surface_to_descriptor[256];
@@ -775,7 +775,7 @@ anv_pipeline_compile_gs(struct anv_pipeline *pipeline,
}
 
if (bin == NULL) {
-  struct brw_gs_prog_data prog_data = { 0, };
+  struct brw_gs_prog_data prog_data = {};
   struct anv_pipeline_binding surface_to_descriptor[256];
   struct anv_pipeline_binding sampler_to_descriptor[256];
 
@@ -853,7 +853,7 @@ anv_pipeline_compile_fs(struct anv_pipeline *pipeline,
}
 
if (bin == NULL) {
-  struct brw_wm_prog_data prog_data = { 0, };
+  struct brw_wm_prog_data prog_data = {};
   struct anv_pipeline_binding surface_to_descriptor[256];
   struct anv_pipeline_binding sampler_to_descriptor[256];
 
@@ -976,7 +976,7 @@ anv_pipeline_compile_cs(struct anv_pipeline *pipeline,
}
 
if (bin == NULL) {
-  struct brw_cs_prog_data prog_data = { 0, };
+  struct brw_cs_prog_data prog_data = {};
   struct anv_pipeline_binding surface_to_descriptor[256];
   struct anv_pipeline_binding sampler_to_descriptor[256];
 
@@ -1277,8 +1277,8 @@ anv_pipeline_init(struct anv_pipeline *pipeline,
 
pipeline->active_stages = 0;
 
-   const VkPipelineShaderStageCreateInfo *pStages[MESA_SHADER_STAGES] = { 0, };
-   struct anv_shader_module *modules[MESA_SHADER_STAGES] = { 0, };
+   const VkPipelineShaderStageCreateInfo *pStages[MESA_SHADER_STAGES] = {};
+   struct anv_shader_module *modules[MESA_SHADER_STAGES] = {};
for (uint32_t i = 0; i < pCreateInfo->stageCount; i++) {
   gl_shader_stage stage = ffs(pCreateInfo->pStages[i].stage) - 1;
   pStages[stage] = >pStages[i];
-- 
2.13.5

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev