[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 <jason at jlekstrand.net>:
> On July 25, 2017 8:16:42 PM "Mun, Gwan-gyeong" <elongbug at gmail.com> 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 <jason at jlekstrand.net>님이 작성:
>>
>>> 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 <elongbug at gmail.com> 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 <elongbug at gmail.com>
>>> > ---
>>> >  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



-- 
Gwan-gyeong Mun