This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [06/77] Make GET_MODE_WIDER return an opt_mode
- From: Jeff Law <law at redhat dot com>
- To: gcc-patches at gcc dot gnu dot org, richard dot sandiford at linaro dot org
- Date: Mon, 28 Aug 2017 11:57:56 -0600
- Subject: Re: [06/77] Make GET_MODE_WIDER return an opt_mode
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=law at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2AA1781235
- References: <8760ewohsv.fsf@linaro.org> <87fue0n317.fsf@linaro.org> <9b296653-5505-8a64-bdde-9ee879f7363e@redhat.com> <87poc2q7xr.fsf@linaro.org>
On 08/11/2017 12:24 PM, Richard Sandiford wrote:
> Jeff Law <law@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.sandiford@linaro.org>
>>> Alan Hayward <alan.hayward@arm.com>
>>> David Sherwood <david.sherwood@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