[04/77] Add FOR_EACH iterators for modes

Message ID 87tw1eqb3d.fsf@linaro.org
State New
Headers show

Commit Message

Richard Sandiford Aug. 11, 2017, 5:16 p.m.
Hi Jeff,

First, many thanks for the reviews!

Jeff Law <law@redhat.com> writes:
> On 07/13/2017 02:39 AM, Richard Sandiford wrote:
>> The new iterators are:
>> 
>> - FOR_EACH_MODE_IN_CLASS: iterate over all the modes in a mode class.
>> 
>> - FOR_EACH_MODE_FROM: iterate over all the modes in a class,
>>   starting at a given mode.
>> 
>> - FOR_EACH_WIDER_MODE: iterate over all the modes in a class,
>>   starting at the next widest mode after a given mode.
>> 
>> - FOR_EACH_2XWIDER_MODE: same, but considering only modes that
>>   are two times wider than the previous mode.
>> 
>> - FOR_EACH_MODE_UNTIL: iterate over all the modes in a class until
>>   a given mode is reached.
>> 
>> - FOR_EACH_MODE: iterate over all the modes in a class between
>>   two given modes, inclusive of the first but not the second.
>> 
>> These help with the stronger type checking added by later patches,
>> since every new mode will be in the same class as the previous one.
>> 
>> 2017-07-13  Richard Sandiford  <richard.sandiford@linaro.org>
>>             Alan Hayward  <alan.hayward@arm.com>
>>             David Sherwood  <david.sherwood@arm.com>
>> 
>> gcc/
>> 	* machmode.h (mode_traits): New structure.
>> 	(get_narrowest_mode): New function.
>> 	(mode_iterator::start): Likewise.
>> 	(mode_iterator::iterate_p): Likewise.
>> 	(mode_iterator::get_wider): Likewise.
>> 	(mode_iterator::get_known_wider): Likewise.
>> 	(mode_iterator::get_2xwider): Likewise.
>> 	(FOR_EACH_MODE_IN_CLASS): New mode iterator.
>> 	(FOR_EACH_MODE): Likewise.
>> 	(FOR_EACH_MODE_FROM): Likewise.
>> 	(FOR_EACH_MODE_UNTIL): Likewise.
>> 	(FOR_EACH_WIDER_MODE): Likewise.
>> 	(FOR_EACH_2XWIDER_MODE): Likewise.
>> 	* builtins.c (expand_builtin_strlen): Use new mode iterators.
>> 	* combine.c (simplify_comparison): Likewise
>> 	* config/i386/i386.c (type_natural_mode): Likewise.
>> 	* cse.c (cse_insn): Likewise.
>> 	* dse.c (find_shift_sequence): Likewise.
>> 	* emit-rtl.c (init_derived_machine_modes): Likewise.
>> 	(init_emit_once): Likewise.
>> 	* explow.c (hard_function_value): Likewise.
>> 	* expmed.c (init_expmed_one_mode): Likewise.
>> 	(extract_fixed_bit_field_1): Likewise.
>> 	(extract_bit_field_1): Likewise.
>> 	(expand_divmod): Likewise.
>> 	(emit_store_flag_1): Likewise.
>> 	* expr.c (init_expr_target): Likewise.
>> 	(convert_move): Likewise.
>> 	(alignment_for_piecewise_move): Likewise.
>> 	(widest_int_mode_for_size): Likewise.
>> 	(emit_block_move_via_movmem): Likewise.
>> 	(copy_blkmode_to_reg): Likewise.
>> 	(set_storage_via_setmem): Likewise.
>> 	(compress_float_constant): Likewise.
>> 	* omp-low.c (omp_clause_aligned_alignment): Likewise.
>> 	* optabs-query.c (get_best_extraction_insn): Likewise.
>> 	* optabs.c (expand_binop): Likewise.
>> 	(expand_twoval_unop): Likewise.
>> 	(expand_twoval_binop): Likewise.
>> 	(widen_leading): Likewise.
>> 	(widen_bswap): Likewise.
>> 	(expand_parity): Likewise.
>> 	(expand_unop): Likewise.
>> 	(prepare_cmp_insn): Likewise.
>> 	(prepare_float_lib_cmp): Likewise.
>> 	(expand_float): Likewise.
>> 	(expand_fix): Likewise.
>> 	(expand_sfix_optab): Likewise.
>> 	* postreload.c (move2add_use_add2_insn): Likewise.
>> 	* reg-stack.c (reg_to_stack): Likewise.
>> 	* reginfo.c (choose_hard_reg_mode): Likewise.
>> 	* rtlanal.c (init_num_sign_bit_copies_in_rep): Likewise.
>> 	* stmt.c (emit_case_decision_tree): Likewise.
>> 	* stor-layout.c (mode_for_size): Likewise.
>> 	(smallest_mode_for_size): Likewise.
>> 	(mode_for_vector): Likewise.
>> 	(finish_bitfield_representative): Likewise.
>> 	* tree-ssa-math-opts.c (target_supports_divmod_p): Likewise.
>> 	* tree-vect-generic.c (type_for_widest_vector_mode): Likewise.
>> 	* tree-vect-stmts.c (vectorizable_conversion): Likewise.
>> 	* var-tracking.c (prepare_call_arguments): Likewise.
>> 
>> gcc/ada/
>> 	* gcc-interface/misc.c (fp_prec_to_size): Use new mode iterators.
>> 	(fp_size_to_prec): Likewise.
>> 
>> gcc/c-family/
>> 	* c-common.c (c_common_fixed_point_type_for_size): Use new mode
>> 	iterators.
>> 	* c-cppbuiltin.c (c_cpp_builtins): Likewise.
> So in some cases I see that we have loops like this:
>
> for (; mode != VOIDmode; mode = GET_WIDER_MODE (mode))
>
> Which iterates from the current mode through all the remaining modes,
> with each iteration using a wider mode than the previous iteration.
> GET_WIDER_MODE is always going to give us something in the same class or
> VOIDmode.
>
> This is getting translated into:
>
> FOR_EACH_MODE_FROM (mode, mode)
>
> The two loops have different steps.  The original steps to a wider mode
> each iteration.  The new one may step to a different mode of the same
> width IIUC.
>
> Am I missing something here?

The new iterators use GET_MODE_WIDER or GET_MODE_2XWIDER to step,
and the patch is only supposed to convert loops that have matching steps.
It wasn't supposed to convert loops that step through modes as numerical
values, but I see now that I fluffed it with:


Sorry about that!  Will fix.

[Maybe in this case the change is safe, since I'm not sure we support
multiple full-integer modes with the same width.  It's still too subtle
for a mechanical change like this though.]

Is that patch OK with that hunk removed?

Thanks,
Richard

> I realize you won't want to use FOR_EACH_WIDER_MODE since that's got a
> different starting point.  But we might want to have an iterator that
> starts at the current mode, but steps to the net wider mode within the
> class.  That would match the existing loops in several places and would
> avoid having to review each one of those use cases to ensure that the
> difference in steps isn't important.
>
> Thoughts?  Am I missing something?
>
> jeff

Comments

Jeff Law Aug. 11, 2017, 5:29 p.m. | #1
On 08/11/2017 11:16 AM, Richard Sandiford wrote:
> Hi Jeff,
> 
> First, many thanks for the reviews!
> 
> Jeff Law <law@redhat.com> writes:
>> On 07/13/2017 02:39 AM, Richard Sandiford wrote:
>>> The new iterators are:
>>>
>>> - FOR_EACH_MODE_IN_CLASS: iterate over all the modes in a mode class.
>>>
>>> - FOR_EACH_MODE_FROM: iterate over all the modes in a class,
>>>   starting at a given mode.
>>>
>>> - FOR_EACH_WIDER_MODE: iterate over all the modes in a class,
>>>   starting at the next widest mode after a given mode.
>>>
>>> - FOR_EACH_2XWIDER_MODE: same, but considering only modes that
>>>   are two times wider than the previous mode.
>>>
>>> - FOR_EACH_MODE_UNTIL: iterate over all the modes in a class until
>>>   a given mode is reached.
>>>
>>> - FOR_EACH_MODE: iterate over all the modes in a class between
>>>   two given modes, inclusive of the first but not the second.
>>>
>>> These help with the stronger type checking added by later patches,
>>> since every new mode will be in the same class as the previous one.
>>>
>>> 2017-07-13  Richard Sandiford  <richard.sandiford@linaro.org>
>>>             Alan Hayward  <alan.hayward@arm.com>
>>>             David Sherwood  <david.sherwood@arm.com>
>>>
>>> gcc/
>>> 	* machmode.h (mode_traits): New structure.
>>> 	(get_narrowest_mode): New function.
>>> 	(mode_iterator::start): Likewise.
>>> 	(mode_iterator::iterate_p): Likewise.
>>> 	(mode_iterator::get_wider): Likewise.
>>> 	(mode_iterator::get_known_wider): Likewise.
>>> 	(mode_iterator::get_2xwider): Likewise.
>>> 	(FOR_EACH_MODE_IN_CLASS): New mode iterator.
>>> 	(FOR_EACH_MODE): Likewise.
>>> 	(FOR_EACH_MODE_FROM): Likewise.
>>> 	(FOR_EACH_MODE_UNTIL): Likewise.
>>> 	(FOR_EACH_WIDER_MODE): Likewise.
>>> 	(FOR_EACH_2XWIDER_MODE): Likewise.
>>> 	* builtins.c (expand_builtin_strlen): Use new mode iterators.
>>> 	* combine.c (simplify_comparison): Likewise
>>> 	* config/i386/i386.c (type_natural_mode): Likewise.
>>> 	* cse.c (cse_insn): Likewise.
>>> 	* dse.c (find_shift_sequence): Likewise.
>>> 	* emit-rtl.c (init_derived_machine_modes): Likewise.
>>> 	(init_emit_once): Likewise.
>>> 	* explow.c (hard_function_value): Likewise.
>>> 	* expmed.c (init_expmed_one_mode): Likewise.
>>> 	(extract_fixed_bit_field_1): Likewise.
>>> 	(extract_bit_field_1): Likewise.
>>> 	(expand_divmod): Likewise.
>>> 	(emit_store_flag_1): Likewise.
>>> 	* expr.c (init_expr_target): Likewise.
>>> 	(convert_move): Likewise.
>>> 	(alignment_for_piecewise_move): Likewise.
>>> 	(widest_int_mode_for_size): Likewise.
>>> 	(emit_block_move_via_movmem): Likewise.
>>> 	(copy_blkmode_to_reg): Likewise.
>>> 	(set_storage_via_setmem): Likewise.
>>> 	(compress_float_constant): Likewise.
>>> 	* omp-low.c (omp_clause_aligned_alignment): Likewise.
>>> 	* optabs-query.c (get_best_extraction_insn): Likewise.
>>> 	* optabs.c (expand_binop): Likewise.
>>> 	(expand_twoval_unop): Likewise.
>>> 	(expand_twoval_binop): Likewise.
>>> 	(widen_leading): Likewise.
>>> 	(widen_bswap): Likewise.
>>> 	(expand_parity): Likewise.
>>> 	(expand_unop): Likewise.
>>> 	(prepare_cmp_insn): Likewise.
>>> 	(prepare_float_lib_cmp): Likewise.
>>> 	(expand_float): Likewise.
>>> 	(expand_fix): Likewise.
>>> 	(expand_sfix_optab): Likewise.
>>> 	* postreload.c (move2add_use_add2_insn): Likewise.
>>> 	* reg-stack.c (reg_to_stack): Likewise.
>>> 	* reginfo.c (choose_hard_reg_mode): Likewise.
>>> 	* rtlanal.c (init_num_sign_bit_copies_in_rep): Likewise.
>>> 	* stmt.c (emit_case_decision_tree): Likewise.
>>> 	* stor-layout.c (mode_for_size): Likewise.
>>> 	(smallest_mode_for_size): Likewise.
>>> 	(mode_for_vector): Likewise.
>>> 	(finish_bitfield_representative): Likewise.
>>> 	* tree-ssa-math-opts.c (target_supports_divmod_p): Likewise.
>>> 	* tree-vect-generic.c (type_for_widest_vector_mode): Likewise.
>>> 	* tree-vect-stmts.c (vectorizable_conversion): Likewise.
>>> 	* var-tracking.c (prepare_call_arguments): Likewise.
>>>
>>> gcc/ada/
>>> 	* gcc-interface/misc.c (fp_prec_to_size): Use new mode iterators.
>>> 	(fp_size_to_prec): Likewise.
>>>
>>> gcc/c-family/
>>> 	* c-common.c (c_common_fixed_point_type_for_size): Use new mode
>>> 	iterators.
>>> 	* c-cppbuiltin.c (c_cpp_builtins): Likewise.
>> So in some cases I see that we have loops like this:
>>
>> for (; mode != VOIDmode; mode = GET_WIDER_MODE (mode))
>>
>> Which iterates from the current mode through all the remaining modes,
>> with each iteration using a wider mode than the previous iteration.
>> GET_WIDER_MODE is always going to give us something in the same class or
>> VOIDmode.
>>
>> This is getting translated into:
>>
>> FOR_EACH_MODE_FROM (mode, mode)
>>
>> The two loops have different steps.  The original steps to a wider mode
>> each iteration.  The new one may step to a different mode of the same
>> width IIUC.
>>
>> Am I missing something here?
> 
> The new iterators use GET_MODE_WIDER or GET_MODE_2XWIDER to step,
> and the patch is only supposed to convert loops that have matching steps.
> It wasn't supposed to convert loops that step through modes as numerical
> values, but I see now that I fluffed it with:
Ah, I was just doing a high level looksie at the conversions first using
the descriptions you gave when I saw what appeared to be the
inconsistency.  Let me repeat that knowing that we're stepping with
GET_MODE_WIDER and friends.


> 
> Index: gcc/expmed.c
> ===================================================================
> --- gcc/expmed.c	2017-07-13 09:17:39.972572368 +0100
> +++ gcc/expmed.c	2017-07-13 09:18:21.531429258 +0100
> @@ -204,8 +204,7 @@ init_expmed_one_mode (struct init_expmed
>  
>    if (SCALAR_INT_MODE_P (mode))
>      {
> -      for (mode_from = MIN_MODE_INT; mode_from <= MAX_MODE_INT;
> -	   mode_from = (machine_mode)(mode_from + 1))
> +      FOR_EACH_MODE_IN_CLASS (mode_from, MODE_INT)
>  	init_expmed_one_conv (all, mode, mode_from, speed);
>      }
>    if (GET_MODE_CLASS (mode) == MODE_INT)
> 
> Sorry about that!  Will fix.
> 
> [Maybe in this case the change is safe, since I'm not sure we support
> multiple full-integer modes with the same width.  It's still too subtle
> for a mechanical change like this though.]
> 
> Is that patch OK with that hunk removed?
Let me walk through a few uses again, then the implementation of the
iterators.  Odds are it's fine, but I should look at it deeper than I
have so far.

jeff

Patch

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	2017-07-13 09:17:39.972572368 +0100
+++ gcc/expmed.c	2017-07-13 09:18:21.531429258 +0100
@@ -204,8 +204,7 @@  init_expmed_one_mode (struct init_expmed
 
   if (SCALAR_INT_MODE_P (mode))
     {
-      for (mode_from = MIN_MODE_INT; mode_from <= MAX_MODE_INT;
-	   mode_from = (machine_mode)(mode_from + 1))
+      FOR_EACH_MODE_IN_CLASS (mode_from, MODE_INT)
 	init_expmed_one_conv (all, mode, mode_from, speed);
     }
   if (GET_MODE_CLASS (mode) == MODE_INT)