Hi Jeff, First, many thanks for the reviews!
Jeff Law <l...@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.sandif...@linaro.org> >> Alan Hayward <alan.hayw...@arm.com> >> David Sherwood <david.sherw...@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: 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? 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