Matt Turner <matts...@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev