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

[Matt Turner]

On Wed, Nov 18, 2015 at 8:15 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> 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.

I agree.

> 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.

I have implemented this locally and I'll Cc you on the patch. The only
other bit I had to do other than what you suggested was add

+   const brw_reg &as_brw_reg() const
+   {
+      return static_cast<const brw_reg &>(*this);
+   }
+
+   brw_reg &as_brw_reg()
+   {
+      return static_cast<brw_reg &>(*this);
+   }

to backend_reg to allow explicit upcasts, e.g., for use as an argument
of of brw_saturate_immediate(enum brw_reg_type type, struct brw_reg
*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

Right, I see the appeal. Part of the problem was that the ternary
arguments were mixed types, which I'm sure one could argue was fine in
the existing code because there was an fs_reg(brw_reg) constructor. In
any case, this is the problem with implicit conversions...

> 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

Interesting. I wasn't aware of that.

> 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.

It was judgmental and I apologize for that. At the same time, I
haven't found another person who disagreed with that opinion. :)

Thanks for the email -- it was really helpful and insightful.