[Mesa-dev] [RFC] intel/isl: Rewrite gen7_choose_image_alignment_el

[Chad Versace]

On Wed 10 May 2017, Kenneth Graunke wrote:
> On Wednesday, May 10, 2017 10:19:46 AM PDT Emil Velikov wrote:
> > On 10 May 2017 at 16:38, Jason Ekstrand <jason at jlekstrand.net> wrote:
> > > On Wed, May 10, 2017 at 8:26 AM, Emil Velikov <emil.l.velikov at gmail.com>
> > > wrote:
> > >>
> > >> Hi Jason,
> > >>
> > >> Humble unrelated question.
> > >>
> > >> On 9 May 2017 at 18:00, Jason Ekstrand <jason at jlekstrand.net> wrote:
> > >>
> > >> > +   if (isl_surf_usage_is_depth(info->usage)) {
> > >> > +      if (info->format == ISL_FORMAT_R16_UNORM) {
> > >> > +         return isl_extent3d(8, 4, 1);
> > >> > +      } else {
> > >> > +         return isl_extent3d(4, 4, 1);
> > >> > +      }
> > >> > +   } else if (isl_surf_usage_is_stencil(info->usage)) {
> > >> > +      return isl_extent3d(8, 8, 1);
> > >> > +   } else if (isl_format_is_compressed(info->format)) {
> > >> > +      /* Compressed formats all have alignment equal to block size. */
> > >> > +      return isl_extent3d(1, 1, 1);
> > >> > +   }
> > >>
> > >> I've seen a handful of constructs like the above.. Is there any reason
> > >> to keep the extra else/curly brackets?
> > >> Something like the following reads a bit easier yet admittedly I'm not
> > >> the person to set the coding style in isl.
> > >
> > >
> > > Does it?
> > >
> > Indeed it does, I would not have bothered otherwise ;-)
> > It's a subjective topic and my contributions here are, ahem, limited,
> > so feel do ignore me.

It most definitely does :)

> I see Jason's point here - the depth/stencil/compressed blocks are
> kind of like cases in a switch.  It's one of the three, and we're
> choosing between them.  I think the code reads fine as is here.
> 
> That said, I almost always prefer Emil's style, FWIW.
> 
>    if (blah)
>       return foo;
> 
>    return bar;
> 
> makes it obvious that something will be returned, as there's an
> clear unconditional return at the top-level.  I especially like this
> style when "foo" is for a special case, and "bar" is the general case.
> 
>    if (blah)
>       return foo;
>    else
>       return bar;
> 
> is a little less clear IMO - you have to recognize that the control flow
> in the inner block will happen, because one of the two blocks will
> always be hit.

I agree with Ken here.

In a chain of early returns...

    if (x0)
        return r0;

    if (x1)
        return r1;

    return r2;

...the code flow is obvious at first glance by all readers, not just the
author. The clarity, I believe, is due to two factors:

    - It resembles the natural human behavior of making choices. For example,
      when you're in grocery aisle trying to choose Ben & Jerry's, the
      thoughts usually proceed as below. Observe how similar it *feels*
      to read code in the early-return style to how it feels to choose
      the perfect pint of ice cream.

            - [Eyes dart to Strawberry Garcia]
            - Do I want strawberry today? ...
            - Nope.

            - [Eyes dart to The Tonight Dough]
            - Do I think Jimmy Fallon is funny?
            - Nope.

            - [Eyes dart to Boom Chocolatta Cookie Core]
            - Do I really want *that* much chocolate in my ice cream?
            - Nope.

            - [Eyes dart to Vanilla]
            - Have I been deceiving myself all these years? Am I, in
              truth, the predictable, boring chump that I have strove so
              hard to avoid becoming? Am I truly the type of person who,
              when confronted with a dizzying array of fanatastical ice
              cream wonders, plays it safe with vanilla, afraid of what
              may lie behind a decision too hasty, too uninformed, too
              bold?
            - Sigh. Yup.

                - [Grab the vanilla]
                - [Return home]

            // You've returned home! The following supermarket scenarios
            // are obviously no longer possible.

            - [Eyes dart to Peanut Butter World]
            ...

            - [Eyes dart to Pumpkin Cheesecake]
            ...

            - [Give up]
            - [Return home empty handed]


    - There is no oppurtunity for a "non-returning" branch to hide among
      the returning branches, like below. Readers (or, at least me)
      intuit this and feel more confidence that they understand the
      code.

        if (blah_blah_blah) {
            return r0;
        } else if (blah_foo_foo_blah) {
            do_stuff;
        } else if (oh_what_did_you_say) {
            return big_thing;
        }

        do_more_stuff();
        return ok_now;


> But it's largely a matter of taste, so...*shrug*.

Not always. The Go code guidelines strongly recommend the "early
return" style for error handling. The guide provides a good explanation.
<https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow>

> If I were writing this, I would probably handle the R16 case as:
> 
>    if (info->format == ISL_FORMAT_R16_UNORM) {
>       return isl_extent3d(8, 4, 1);
> 
>    return isl_extent3d(4, 4, 1);
> 
> but leave the rest as is.  Totally up to you though, Jason.

Anyway, I've stated my preference. Either way, this patch is a large
improvement over my original isl code.

Reviewed-by: Chad Versace <chadversary at chromium.org>