[Mesa-dev] [PATCH] i965/fs: Replace nested ternary with if ladder.

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> Since the types of the expression were
>
>    bool ? src_reg : (bool ? brw_reg : brw_reg)
>
> the result of the second (nested) ternary would be implicitly
> converted to a src_reg by the src_reg(struct brw_reg) constructor. I.e.,
>
>    bool ? src_reg : src_reg(bool ? brw_reg : brw_reg)
>
> In the next patch, I make backend_reg (the parent of src_reg) inherit
> from brw_reg, which changes this expression to return brw_reg, which
> throws away any fields that exist in the classes derived from brw_reg.
> I.e.,
>
>    src_reg(bool ? brw_reg(src_reg) : bool ? brw_reg : brw_reg)
>

The fundamental problem here has nothing to do with ternary operators,
but with the fact that you made backend_reg inherit from brw_reg even
though there is no is-a relationship -- More precisely, the Liskov
substitution principle doesn't hold: Pass a valid program written in
terms of brw_reg (e.g. brw_MOV()) an fs_reg (or similarly any of the
vec4 register objects) and you've got a bug (AKA object slicing).  The
reason is that fs_regs have additional structure and restrictions that a
valid brw_reg program won't take into account (e.g. additional offset,
stride, reladdr), and even the methods shared with the base brw_reg
class have subtly different semantics.  A proper conversion from fs_reg
to brw_reg amounts to more than just slicing off the additional
structure of the fs_reg (c.f. brw_reg_from_fs_reg()), what points at
there being an implemented-in-terms-of relationship rather than an is-a
relationship.

The typical approach to solve this problem is use private inheritance
and explicitly expose the fields from the base class you want to be
public using "using" directives.  That will prevent implicit (and
generally incorrect) conversion of register subclasses into brw_reg.

Another somewhat more wordy approach is use aggregation and expose any
interesting fields from the brw_reg using accessors, or use completely
unrelated representations for backend_reg and brw_reg and perform the
conversion manually (kind of like was done previously, but replacing the
fixed_hw_reg thing with precisely the subset of brw_reg fields required
to represent HW_REGs and immediates orthogonally with respect to the
remaining backend_reg fields).

> Generally this code was gross, and wasn't actually shorter or easier to
> read than an if ladder.

Generally I find it gross to split up a pure expression into separate
statements even though there's no control-flow relationship intended.
The widely used pattern:

| condition0 ? expression0 :
| condition1 ? expression1 :
| /* ... */
| conditionN ? expressionN :
| default-expression

is at least as readable as the equivalent if-ladder (although this point
is highly subjective) and frequently saves you from introducing mutable
variables that only ever need to be assigned once and after that point
remain constant even though they're not explicitly marked as constants
(what obscures the dataflow of the program -- This last point is
probably not subjective at all, but it doesn't apply to this particular
example).  In other cases chained ternary operators can simplify code by
allowing the factorization of duplicated code like:

| p ? f(x) :
| q ? f(y) :
| /* ... */ :
| f(z)

  ->

| f(p ? x :
|   q ? y :
|   /* ... */ :
|   z)

In this particular case the duplicated "f" factor you added is just a
trivial return statement so this benefit is not too important in this
case either, and you're left with the purely subjective matter of which
is the most readable -- In doubt I tend to prefer ternary operators
instinctively when the alternatives are pure expressions simply for
consistency and brevity.  You may feel otherwise and I'm not going to
complain about this change because the difference is purely subjective
-- But for that same reason calling this "gross" seems like a rather
gross overstatement to me.

> ---
>  src/mesa/drivers/dri/i965/brw_fs_builder.h | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_builder.h b/src/mesa/drivers/dri/i965/brw_fs_builder.h
> index f121f34..d5763f6 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_builder.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs_builder.h
> @@ -224,12 +224,13 @@ namespace brw {
>        src_reg
>        sample_mask_reg() const
>        {
> -         const bool uses_kill =
> -            (shader->stage == MESA_SHADER_FRAGMENT &&
> -             ((brw_wm_prog_data *)shader->stage_prog_data)->uses_kill);
> -         return (shader->stage != MESA_SHADER_FRAGMENT ? src_reg(0xffff) :
> -                 uses_kill ? brw_flag_reg(0, 1) :
> -                 retype(brw_vec1_grf(1, 7), BRW_REGISTER_TYPE_UD));
> +         if (shader->stage != MESA_SHADER_FRAGMENT) {
> +            return src_reg(0xffff);
> +         } else if (((brw_wm_prog_data *)shader->stage_prog_data)->uses_kill) {
> +            return brw_flag_reg(0, 1);
> +         } else {
> +            return retype(brw_vec1_grf(1, 7), BRW_REGISTER_TYPE_UD);
> +         }
>        }
>  
>        /**
> -- 
> 2.4.9
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151118/446ab96a/attachment.sig>