Re: [Mesa-dev] [PATCH] genxml: Remove a redundant identical code for different branches

[Emil Velikov]

On 2 August 2017 at 10:17, Mun, Gwan-gyeong  wrote:
> Hi Jason,
>
> thanks for your kind explanation.
>
> I has totally understood your intention.
>
> ( I don't mean to bother you,  at first I just wanted to silence of
> below coverity warning.
> ---
> Identical code for different branches (IDENTICAL_BRANCHES)
> identical_branches: The same code is executed regardless of whether
> devinfo->is_haswell is true, because the 'then' and 'else' branches
> are identical. Should one of the branches be modified, or the entire
> 'if' statement replaced?
> ---
> }
>
Simply can change "Classification" to "Intentional" or "False Positive" ;-)

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

Re: [Mesa-dev] [PATCH] genxml: Remove a redundant identical code for different branches

[Mun, Gwan-gyeong]

Hi Jason,

thanks for your kind explanation.

I has totally understood your intention.

( I don't mean to bother you,  at first I just wanted to silence of
below coverity warning.
---
Identical code for different branches (IDENTICAL_BRANCHES)
identical_branches: The same code is executed regardless of whether
devinfo->is_haswell is true, because the 'then' and 'else' branches
are identical. Should one of the branches be modified, or the entire
'if' statement replaced?
---
}

Best regards,

Gwan-gyeong.

2017-07-27 4:30 GMT+09:00 Jason Ekstrand :
> On July 25, 2017 8:16:42 PM "Mun, Gwan-gyeong"  wrote:
>
>> Hi Jason,
>> You are right, as you commented, compilers can eliminate these
>> redundancies
>> easy.
>> However I think we don't need to generate redundant codes.
>
>
> The approach we generally take with generators like this is to not really
> care too much what the generated C code looks like at that much so long as
> it compiles down to what we want.  We are far more concerned with keeping
> the generator itself as simple and easy to maintain as possible.  If we make
> the C compiler do a little more work, that's considered an acceptable loss
> so long as the final compiled binary is good.  The part of the code which is
> maintained by humans is the generator so the ease of maintenance of the
> generator is more important than the generated C code.
>
> This patch makes the generator more complicated just to improve the
> generated C even though it compiles to the same binary.  As such, this patch
> makes the codebase *less* maintainable with no improvement to the generated
> binary.  This is not something we want to do.
>
> --Jason
>
>
>> Best regards,
>> Gwan-gyeong
>>
>> 2017년 7월 26일 (수) 오전 12:34, Jason Ekstrand 님이 작성:
>>
>>> Does the redundancy ends up mattering in any way?  A decent optimizing
>>> compiler should easily be able to get rid of that for you.
>>>
>>> --Jason
>>>
>>>
>>> On July 25, 2017 2:51:31 AM Gwan-gyeong Mun  wrote:
>>>
>>> > Before, it generates functions like this,
>>> >
>>> > static inline uint32_t ATTRIBUTE_PURE
>>> > RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info
>>> *devinfo)
>>> > {
>>> >switch (devinfo->gen) {
>>> >case 10: return 384;
>>> >case 9: return 384;
>>> >case 8: return 255;
>>> >case 7:
>>> >   if (devinfo->is_haswell) {
>>> >  return 255;
>>> >   } else {
>>> >  return 255;
>>> >   }
>>> >case 6: return 0;
>>> >case 5: return 0;
>>> >case 4:
>>> >   if (devinfo->is_g4x) {
>>> >  return 0;
>>> >   } else {
>>> >  return 0;
>>> >   }
>>> >default:
>>> >   unreachable("Invalid hardware generation");
>>> >}
>>> > }
>>> >
>>> > After, it generates fuctions without a redundant identical code for
>>> different
>>> > branches.
>>> >
>>> > static inline uint32_t ATTRIBUTE_PURE
>>> > RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info
>>> *devinfo)
>>> > {
>>> >switch (devinfo->gen) {
>>> >case 10: return 384;
>>> >case 9: return 384;
>>> >case 8: return 255;
>>> >case 7: return 255;
>>> >case 6: return 0;
>>> >case 5: return 0;
>>> >case 4: return 0;
>>> >default:
>>> >   unreachable("Invalid hardware generation");
>>> >}
>>> > }
>>> >
>>> > Signed-off-by: Mun Gwan-gyeong 
>>> > ---
>>> >  src/intel/genxml/gen_bits_header.py | 8 
>>> >  1 file changed, 8 insertions(+)
>>> >
>>> > diff --git a/src/intel/genxml/gen_bits_header.py
>>> > b/src/intel/genxml/gen_bits_header.py
>>> > index 1b3504073b..8084facdb7 100644
>>> > --- a/src/intel/genxml/gen_bits_header.py
>>> > +++ b/src/intel/genxml/gen_bits_header.py
>>> > @@ -83,20 +83,28 @@ ${item.token_name}_${prop}(const struct
>>> gen_device_info
>>> > *devinfo)
>>> > case 10: return ${item.get_prop(prop, 10)};
>>> > case 9: return ${item.get_prop(prop, 9)};
>>> > case 8: return ${item.get_prop(prop, 8)};
>>> > +% if item.get_prop(prop, 7) == item.get_prop(prop, 7.5):
>>> > +   case 7: return ${item.get_prop(prop, 7)};
>>> > +% else:
>>> > case 7:
>>> >if (devinfo->is_haswell) {
>>> >   return ${item.get_prop(prop, 7.5)};
>>> >} else {
>>> >   return ${item.get_prop(prop, 7)};
>>> >}
>>> > +% endif
>>> > case 6: return ${item.get_prop(prop, 6)};
>>> > case 5: return ${item.get_prop(prop, 5)};
>>> > +% if item.get_prop(prop, 4) == item.get_prop(prop, 4.5):
>>> > +   case 4: return ${item.get_prop(prop, 4)};
>>> > +% else:
>>> > case 4:
>>> >if (devinfo->is_g4x) {
>>> > 

Re: [Mesa-dev] [PATCH] genxml: Remove a redundant identical code for different branches

[Jason Ekstrand]

On July 25, 2017 8:16:42 PM "Mun, Gwan-gyeong"  wrote:


Hi Jason,
You are right, as you commented, compilers can eliminate these redundancies
easy.
However I think we don't need to generate redundant codes.


The approach we generally take with generators like this is to not really 
care too much what the generated C code looks like at that much so long as 
it compiles down to what we want.  We are far more concerned with keeping 
the generator itself as simple and easy to maintain as possible.  If we 
make the C compiler do a little more work, that's considered an acceptable 
loss so long as the final compiled binary is good.  The part of the code 
which is maintained by humans is the generator so the ease of maintenance 
of the generator is more important than the generated C code.


This patch makes the generator more complicated just to improve the 
generated C even though it compiles to the same binary.  As such, this 
patch makes the codebase *less* maintainable with no improvement to the 
generated binary.  This is not something we want to do.


--Jason


Best regards,
Gwan-gyeong

2017년 7월 26일 (수) 오전 12:34, Jason Ekstrand 님이 작성:


Does the redundancy ends up mattering in any way?  A decent optimizing
compiler should easily be able to get rid of that for you.

--Jason


On July 25, 2017 2:51:31 AM Gwan-gyeong Mun  wrote:

> Before, it generates functions like this,
>
> static inline uint32_t ATTRIBUTE_PURE
> RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info
*devinfo)
> {
>switch (devinfo->gen) {
>case 10: return 384;
>case 9: return 384;
>case 8: return 255;
>case 7:
>   if (devinfo->is_haswell) {
>  return 255;
>   } else {
>  return 255;
>   }
>case 6: return 0;
>case 5: return 0;
>case 4:
>   if (devinfo->is_g4x) {
>  return 0;
>   } else {
>  return 0;
>   }
>default:
>   unreachable("Invalid hardware generation");
>}
> }
>
> After, it generates fuctions without a redundant identical code for
different
> branches.
>
> static inline uint32_t ATTRIBUTE_PURE
> RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info
*devinfo)
> {
>switch (devinfo->gen) {
>case 10: return 384;
>case 9: return 384;
>case 8: return 255;
>case 7: return 255;
>case 6: return 0;
>case 5: return 0;
>case 4: return 0;
>default:
>   unreachable("Invalid hardware generation");
>}
> }
>
> Signed-off-by: Mun Gwan-gyeong 
> ---
>  src/intel/genxml/gen_bits_header.py | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/intel/genxml/gen_bits_header.py
> b/src/intel/genxml/gen_bits_header.py
> index 1b3504073b..8084facdb7 100644
> --- a/src/intel/genxml/gen_bits_header.py
> +++ b/src/intel/genxml/gen_bits_header.py
> @@ -83,20 +83,28 @@ ${item.token_name}_${prop}(const struct
gen_device_info
> *devinfo)
> case 10: return ${item.get_prop(prop, 10)};
> case 9: return ${item.get_prop(prop, 9)};
> case 8: return ${item.get_prop(prop, 8)};
> +% if item.get_prop(prop, 7) == item.get_prop(prop, 7.5):
> +   case 7: return ${item.get_prop(prop, 7)};
> +% else:
> case 7:
>if (devinfo->is_haswell) {
>   return ${item.get_prop(prop, 7.5)};
>} else {
>   return ${item.get_prop(prop, 7)};
>}
> +% endif
> case 6: return ${item.get_prop(prop, 6)};
> case 5: return ${item.get_prop(prop, 5)};
> +% if item.get_prop(prop, 4) == item.get_prop(prop, 4.5):
> +   case 4: return ${item.get_prop(prop, 4)};
> +% else:
> case 4:
>if (devinfo->is_g4x) {
>   return ${item.get_prop(prop, 4.5)};
>} else {
>   return ${item.get_prop(prop, 4)};
>}
> +% endif
> default:
>unreachable("Invalid hardware generation");
> }
> --
> 2.13.3
>


--

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

Re: [Mesa-dev] [PATCH] genxml: Remove a redundant identical code for different branches

[Mun, Gwan-gyeong]

Hi Jason,
You are right, as you commented, compilers can eliminate these redundancies
easy.
However I think we don't need to generate redundant codes.

Best regards,
Gwan-gyeong

2017년 7월 26일 (수) 오전 12:34, Jason Ekstrand 님이 작성:

> Does the redundancy ends up mattering in any way?  A decent optimizing
> compiler should easily be able to get rid of that for you.
>
> --Jason
>
>
> On July 25, 2017 2:51:31 AM Gwan-gyeong Mun  wrote:
>
> > Before, it generates functions like this,
> >
> > static inline uint32_t ATTRIBUTE_PURE
> > RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info
> *devinfo)
> > {
> >switch (devinfo->gen) {
> >case 10: return 384;
> >case 9: return 384;
> >case 8: return 255;
> >case 7:
> >   if (devinfo->is_haswell) {
> >  return 255;
> >   } else {
> >  return 255;
> >   }
> >case 6: return 0;
> >case 5: return 0;
> >case 4:
> >   if (devinfo->is_g4x) {
> >  return 0;
> >   } else {
> >  return 0;
> >   }
> >default:
> >   unreachable("Invalid hardware generation");
> >}
> > }
> >
> > After, it generates fuctions without a redundant identical code for
> different
> > branches.
> >
> > static inline uint32_t ATTRIBUTE_PURE
> > RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info
> *devinfo)
> > {
> >switch (devinfo->gen) {
> >case 10: return 384;
> >case 9: return 384;
> >case 8: return 255;
> >case 7: return 255;
> >case 6: return 0;
> >case 5: return 0;
> >case 4: return 0;
> >default:
> >   unreachable("Invalid hardware generation");
> >}
> > }
> >
> > Signed-off-by: Mun Gwan-gyeong 
> > ---
> >  src/intel/genxml/gen_bits_header.py | 8 
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/intel/genxml/gen_bits_header.py
> > b/src/intel/genxml/gen_bits_header.py
> > index 1b3504073b..8084facdb7 100644
> > --- a/src/intel/genxml/gen_bits_header.py
> > +++ b/src/intel/genxml/gen_bits_header.py
> > @@ -83,20 +83,28 @@ ${item.token_name}_${prop}(const struct
> gen_device_info
> > *devinfo)
> > case 10: return ${item.get_prop(prop, 10)};
> > case 9: return ${item.get_prop(prop, 9)};
> > case 8: return ${item.get_prop(prop, 8)};
> > +% if item.get_prop(prop, 7) == item.get_prop(prop, 7.5):
> > +   case 7: return ${item.get_prop(prop, 7)};
> > +% else:
> > case 7:
> >if (devinfo->is_haswell) {
> >   return ${item.get_prop(prop, 7.5)};
> >} else {
> >   return ${item.get_prop(prop, 7)};
> >}
> > +% endif
> > case 6: return ${item.get_prop(prop, 6)};
> > case 5: return ${item.get_prop(prop, 5)};
> > +% if item.get_prop(prop, 4) == item.get_prop(prop, 4.5):
> > +   case 4: return ${item.get_prop(prop, 4)};
> > +% else:
> > case 4:
> >if (devinfo->is_g4x) {
> >   return ${item.get_prop(prop, 4.5)};
> >} else {
> >   return ${item.get_prop(prop, 4)};
> >}
> > +% endif
> > default:
> >unreachable("Invalid hardware generation");
> > }
> > --
> > 2.13.3
> >
>
>
> --
Gwan-gyeong Mun
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] genxml: Remove a redundant identical code for different branches

[Jason Ekstrand]

Does the redundancy ends up mattering in any way?  A decent optimizing 
compiler should easily be able to get rid of that for you.


--Jason


On July 25, 2017 2:51:31 AM Gwan-gyeong Mun  wrote:


Before, it generates functions like this,

static inline uint32_t ATTRIBUTE_PURE
RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info *devinfo)
{
   switch (devinfo->gen) {
   case 10: return 384;
   case 9: return 384;
   case 8: return 255;
   case 7:
  if (devinfo->is_haswell) {
 return 255;
  } else {
 return 255;
  }
   case 6: return 0;
   case 5: return 0;
   case 4:
  if (devinfo->is_g4x) {
 return 0;
  } else {
 return 0;
  }
   default:
  unreachable("Invalid hardware generation");
   }
}

After, it generates fuctions without a redundant identical code for different
branches.

static inline uint32_t ATTRIBUTE_PURE
RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info *devinfo)
{
   switch (devinfo->gen) {
   case 10: return 384;
   case 9: return 384;
   case 8: return 255;
   case 7: return 255;
   case 6: return 0;
   case 5: return 0;
   case 4: return 0;
   default:
  unreachable("Invalid hardware generation");
   }
}

Signed-off-by: Mun Gwan-gyeong 
---
 src/intel/genxml/gen_bits_header.py | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/intel/genxml/gen_bits_header.py 
b/src/intel/genxml/gen_bits_header.py

index 1b3504073b..8084facdb7 100644
--- a/src/intel/genxml/gen_bits_header.py
+++ b/src/intel/genxml/gen_bits_header.py
@@ -83,20 +83,28 @@ ${item.token_name}_${prop}(const struct gen_device_info 
*devinfo)

case 10: return ${item.get_prop(prop, 10)};
case 9: return ${item.get_prop(prop, 9)};
case 8: return ${item.get_prop(prop, 8)};
+% if item.get_prop(prop, 7) == item.get_prop(prop, 7.5):
+   case 7: return ${item.get_prop(prop, 7)};
+% else:
case 7:
   if (devinfo->is_haswell) {
  return ${item.get_prop(prop, 7.5)};
   } else {
  return ${item.get_prop(prop, 7)};
   }
+% endif
case 6: return ${item.get_prop(prop, 6)};
case 5: return ${item.get_prop(prop, 5)};
+% if item.get_prop(prop, 4) == item.get_prop(prop, 4.5):
+   case 4: return ${item.get_prop(prop, 4)};
+% else:
case 4:
   if (devinfo->is_g4x) {
  return ${item.get_prop(prop, 4.5)};
   } else {
  return ${item.get_prop(prop, 4)};
   }
+% endif
default:
   unreachable("Invalid hardware generation");
}
--
2.13.3




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

[Mesa-dev] [PATCH] genxml: Remove a redundant identical code for different branches

[Gwan-gyeong Mun]

Before, it generates functions like this,

static inline uint32_t ATTRIBUTE_PURE
RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info *devinfo)
{
   switch (devinfo->gen) {
   case 10: return 384;
   case 9: return 384;
   case 8: return 255;
   case 7:
  if (devinfo->is_haswell) {
 return 255;
  } else {
 return 255;
  }
   case 6: return 0;
   case 5: return 0;
   case 4:
  if (devinfo->is_g4x) {
 return 0;
  } else {
 return 0;
  }
   default:
  unreachable("Invalid hardware generation");
   }
}

After, it generates fuctions without a redundant identical code for different
branches.

static inline uint32_t ATTRIBUTE_PURE
RENDER_SURFACE_STATE_RedClearColor_start(const struct gen_device_info *devinfo)
{
   switch (devinfo->gen) {
   case 10: return 384;
   case 9: return 384;
   case 8: return 255;
   case 7: return 255;
   case 6: return 0;
   case 5: return 0;
   case 4: return 0;
   default:
  unreachable("Invalid hardware generation");
   }
}

Signed-off-by: Mun Gwan-gyeong 
---
 src/intel/genxml/gen_bits_header.py | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/intel/genxml/gen_bits_header.py 
b/src/intel/genxml/gen_bits_header.py
index 1b3504073b..8084facdb7 100644
--- a/src/intel/genxml/gen_bits_header.py
+++ b/src/intel/genxml/gen_bits_header.py
@@ -83,20 +83,28 @@ ${item.token_name}_${prop}(const struct gen_device_info 
*devinfo)
case 10: return ${item.get_prop(prop, 10)};
case 9: return ${item.get_prop(prop, 9)};
case 8: return ${item.get_prop(prop, 8)};
+% if item.get_prop(prop, 7) == item.get_prop(prop, 7.5):
+   case 7: return ${item.get_prop(prop, 7)};
+% else:
case 7:
   if (devinfo->is_haswell) {
  return ${item.get_prop(prop, 7.5)};
   } else {
  return ${item.get_prop(prop, 7)};
   }
+% endif
case 6: return ${item.get_prop(prop, 6)};
case 5: return ${item.get_prop(prop, 5)};
+% if item.get_prop(prop, 4) == item.get_prop(prop, 4.5):
+   case 4: return ${item.get_prop(prop, 4)};
+% else:
case 4:
   if (devinfo->is_g4x) {
  return ${item.get_prop(prop, 4.5)};
   } else {
  return ${item.get_prop(prop, 4)};
   }
+% endif
default:
   unreachable("Invalid hardware generation");
}
-- 
2.13.3

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