Jeff Law <l...@redhat.com> writes: > On 08/11/2017 12:24 PM, Richard Sandiford wrote: >> Jeff Law <l...@redhat.com> writes: >>> On 07/13/2017 02:40 AM, Richard Sandiford wrote: >>>> GET_MODE_WIDER previously returned VOIDmode if no wider mode existed. >>>> That would cause problems with stricter mode classes, since VOIDmode >>>> isn't for example a valid scalar integer or floating-point mode. >>>> This patch instead makes it return a new opt_mode<T> class, which >>>> holds either a T or nothing. >>>> >>>> 2017-07-13 Richard Sandiford <richard.sandif...@linaro.org> >>>> Alan Hayward <alan.hayw...@arm.com> >>>> David Sherwood <david.sherw...@arm.com> >>>> >>>> gcc/ >>>> * coretypes.h (opt_mode): New class. >>>> * machmode.h (opt_mode): Likewise. >>>> (opt_mode::else_void): New function. >>>> (opt_mode::operator *): Likewise. >>>> (opt_mode::exists): Likewise. >>>> (GET_MODE_WIDER_MODE): Turn into a function and return an opt_mode. >>>> (GET_MODE_2XWIDER_MODE): Likewise. >>>> (mode_iterator::get_wider): Update accordingly. >>>> (mode_iterator::get_2xwider): Likewise. >>>> (mode_iterator::get_known_wider): Likewise, turning into a template. >>>> * combine.c (make_extraction): Update use of GET_MODE_WIDER_MODE, >>>> forcing a wider mode to exist. >>>> * config/cr16/cr16.h (LONG_REG_P): Likewise. >>>> * rtlanal.c (init_num_sign_bit_copies_in_rep): Likewise. >>>> * config/c6x/c6x.c (c6x_rtx_costs): Update use of >>>> GET_MODE_2XWIDER_MODE, forcing a wider mode to exist. >>>> * lower-subreg.c (init_lower_subreg): Likewise. >>>> * optabs-libfuncs.c (init_sync_libfuncs_1): Likewise, but not >>>> on the final iteration. >>>> * config/i386/i386.c (ix86_expand_set_or_movmem): Check whether >>>> a wider mode exists before asking for a move pattern. >>>> (get_mode_wider_vector): Update use of GET_MODE_WIDER_MODE, >>>> forcing a wider mode to exist. >>>> (expand_vselect_vconcat): Update use of GET_MODE_2XWIDER_MODE, >>>> returning false if no such mode exists. >>>> * config/ia64/ia64.c (expand_vselect_vconcat): Likewise. >>>> * config/mips/mips.c (mips_expand_vselect_vconcat): Likewise. >>>> * expmed.c (init_expmed_one_mode): Update use of GET_MODE_WIDER_MODE. >>>> Avoid checking for a MODE_INT if we already know the mode is not a >>>> SCALAR_INT_MODE_P. >>>> (extract_high_half): Update use of GET_MODE_WIDER_MODE, >>>> forcing a wider mode to exist. >>>> (expmed_mult_highpart_optab): Likewise. >>>> (expmed_mult_highpart): Likewise. >>>> * expr.c (expand_expr_real_2): Update use of GET_MODE_WIDER_MODE, >>>> using else_void. >>>> * lto-streamer-in.c (lto_input_mode_table): Likewise. >>>> * optabs-query.c (find_widening_optab_handler_and_mode): Likewise. >>>> * stor-layout.c (bit_field_mode_iterator::next_mode): Likewise. >>>> * internal-fn.c (expand_mul_overflow): Update use of >>>> GET_MODE_2XWIDER_MODE. >>>> * omp-low.c (omp_clause_aligned_alignment): Likewise. >>>> * tree-ssa-math-opts.c (convert_mult_to_widen): Update use of >>>> GET_MODE_WIDER_MODE. >>>> (convert_plusminus_to_widen): Likewise. >>>> * tree-switch-conversion.c (array_value_type): Likewise. >>>> * var-tracking.c (emit_note_insn_var_location): Likewise. >>>> * tree-vrp.c (simplify_float_conversion_using_ranges): Likewise. >>>> Return false inside rather than outside the loop if no wider mode >>>> exists >>>> * optabs.c (expand_binop): Update use of GET_MODE_WIDER_MODE >>>> and GET_MODE_2XWIDER_MODE >>>> (can_compare_p): Use else_void. >>>> * gdbhooks.py (OptMachineModePrinter): New class. >>>> (build_pretty_printer): Use it for opt_mode. >>>> >>>> gcc/ada/ >>>> * gcc-interface/decl.c (validate_size): Update use of >>>> GET_MODE_WIDER_MODE, forcing a wider mode to exist. >>> I'm not a big fan of the API here, particularly using operator* to >>> handle asserting the mode exists. I'd prefer to just use a member >>> function rather than overloading operator*. >>> >>> What's the rationale behind using operator* to imply the assertion? >>> >>> THe changes themsleves look fine, it's really just a question of the API >>> we present. >> >> The original idea was to make opt_mode look pointer-ish, so that >> the dyn_cast <...> result could be used in the same way as for >> dyn_cast <gassign *> etc. The first cut therefore had operator bool () >> to test whether there was a mode and operator * to dereference it. >> >> However, operator bool () created various subtle problems (as it always >> seems to) so we dropped it in favour of exists (). I was neutral >> on whether we should keep '*' or switch to a function, so in the >> end the status quo won out. I'm happy to change it to a named >> accessor though. >> >> Any better ideas than "get ()" for the name? Maybe something >> to emphasis that it is asserting for non-nullness/non-emptiness >> (which '*' does implicitly)? > > Yea, when I was reading the first few patches it felt like you trying >to do a pointer-ish API. > > I think we should avoid the operator overload. It's not real obvious > what's going on and I think our guidelines generally discourage operator > overloading. Sadly I think it's going to require a lot of mechanical > changes, but better to do it now than go back later and do it.
That's OK, it should be a trivial update. > As for the name, get_nonvoid? Ugh. Not sure. Open to suggestions. I'd rather avoid "nonvoid", since the use of VOIDmode for "no mode" is really an implementation detail in things like opt_mode <scalar_int_mode>. Other possiblities might be: - require - demand - mode - get_mode - require_mode - demand_mode - else_fail (to go with else_void and else_blk) - noelse Thanks, Richard
