[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" <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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170726/db308070/attachment-0001.html>