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. As for the name, get_nonvoid? Ugh. Not sure. Open to suggestions. jeff
