Re: [PATCH] early outs for functions in rs6000.cc
Hi,
David Edelsohn writes:
>
> On Tue, Oct 10, 2023 at 9:29 PM Jiufu Guo wrote:
>
> Hi,
>
> There are some piece of code like below in rs6000.cc:
>
> ...
> if (xx)
> return x;
> else if (yy)
> return y;
> ... //else if chain
> else
> return d;
>
> Using early outs would be more preferable for this kind of code.
>
> Why?
>
> This seems like premature optimization.
It would be a code style preference.
Early out would be useful especially for the deep nested 'if'
statements.
While, sometimes, exiting at multiple places may cause some
confusing and hard to refactor.
The 'if-else's of this patch in rs6000.cc are not nested too
much. The 'if-else' of this patch is not very hard to read.
While, early outs of this patch may still give some benefits:
- It make shorter indentation, especailly for the last 'else'
or last 'else if' statements. e.g. the changes in
determine_suggested_unroll_factor and legitimate_lo_sum_address_p.
- Some code are partial early-return:
"if(x) return x; if (y) return y; else return z;"
- It may help to read the condition code. When reading each return,
"if(){xx ;return;}", only to make sure current 'if' is correct,
and do not need to care about the relations between 'if's.
I post this patch just wondering if it is also a prefer for guys.
BR,
Jeff (Jiufu Guo)
>
> Thanks, David
>
>
> The whole function rs6000_emit_set_long_const and num_insns_constant_gpr
> are refined. And similar code are also restuctured in rs6000.cc.
>
> This patch pass bootstrap and regtest on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu Guo)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (vspltis_shifted): Adopt early outs.
> (rs6000_cost_data::determine_suggested_unroll_factor): Likewise.
> (num_insns_constant_gpr): Likewise.
> (easy_altivec_constant): Likewise.
> (output_vec_const_move): Likewise.
> (rs6000_expand_vector_set): Likewise.
> (rs6000_legitimize_address): Likewise.
> (rs6000_emit_set_long_const): Likewise.
> (rs6000_preferred_reload_class): Likewise.
> (rs6000_can_change_mode_class): Likewise.
> (rs6000_output_move_128bit): Likewise.
> (rs6000_emit_vector_cond_expr): Likewise.
> (adjacent_mem_locations): Likewise.
> (is_fusable_store): Likewise.
> (insn_terminates_group_p): Likewise.
> (rs6000_elf_reloc_rw_mask): Likewise.
> (rs6000_rtx_costs): Likewise.
> (rs6000_scalar_mode_supported_p): Likewise.
> (rs6000_update_ipa_fn_target_info): Likewise.
> (reg_to_non_prefixed): Likewise.
> (rs6000_split_logical_inner): Likewise.
> (rs6000_opaque_type_invalid_use_p): Likewise.
>
> ---
> gcc/config/rs6000/rs6000.cc | 495 ++--
> 1 file changed, 249 insertions(+), 246 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index d10d22a5816..43681d9eb08 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -5522,24 +5522,22 @@ rs6000_cost_data::determine_suggested_unroll_factor
> (loop_vec_info loop_vinfo)
> to vectorize it with the unrolled VF any more if the actual iteration
> count is in between. */
> return 1;
> - else
> -{
> - unsigned int epil_niter_unr = est_niter % unrolled_vf;
> - unsigned int epil_niter = est_niter % vf;
> - /* Even if we have partial vector support, it can be still inefficent
> -to calculate the length when the iteration count is unknown, so
> -only expect it's good to unroll when the epilogue iteration count
> -is not bigger than VF (only one time length calculation). */
> - if (LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> - && epil_niter_unr <= vf)
> - return uf;
> - /* Without partial vector support, conservatively unroll this when
> -the epilogue iteration count is less than the original one
> -(epilogue execution time wouldn't be longer than before). */
> - else if (!LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> - && epil_niter_unr <= epil_niter)
> - return uf;
> -}
> +
> + unsigned int epil_niter_unr = est_niter % unrolled_vf;
> + unsigned int epil_niter = est_niter % vf;
> + /* Even if we have partial vector support, it can be still inefficent
> + to calculate the length when the iteration count is unknown, so
> + only expect it's good to unroll when the epilogue iteration count
> + is not bigger than VF (only one time length calculation). */
> + if (LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo) && epil_niter_unr
> <= vf)
> +return uf;
> +
> + /* Without partial vector support, conservatively unroll this when
> + the epilogue iteration count is less than the original one
> + (epilogue
Re: [PATCH] early outs for functions in rs6000.cc
On Tue, Oct 10, 2023 at 9:29 PM Jiufu Guo wrote:
> Hi,
>
> There are some piece of code like below in rs6000.cc:
>
> ...
> if (xx)
>return x;
> else if (yy)
>return y;
> ... //else if chain
> else
>return d;
>
> Using early outs would be more preferable for this kind of code.
>
Why?
This seems like premature optimization.
Thanks, David
>
> The whole function rs6000_emit_set_long_const and num_insns_constant_gpr
> are refined. And similar code are also restuctured in rs6000.cc.
>
> This patch pass bootstrap and regtest on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu Guo)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (vspltis_shifted): Adopt early outs.
> (rs6000_cost_data::determine_suggested_unroll_factor): Likewise.
> (num_insns_constant_gpr): Likewise.
> (easy_altivec_constant): Likewise.
> (output_vec_const_move): Likewise.
> (rs6000_expand_vector_set): Likewise.
> (rs6000_legitimize_address): Likewise.
> (rs6000_emit_set_long_const): Likewise.
> (rs6000_preferred_reload_class): Likewise.
> (rs6000_can_change_mode_class): Likewise.
> (rs6000_output_move_128bit): Likewise.
> (rs6000_emit_vector_cond_expr): Likewise.
> (adjacent_mem_locations): Likewise.
> (is_fusable_store): Likewise.
> (insn_terminates_group_p): Likewise.
> (rs6000_elf_reloc_rw_mask): Likewise.
> (rs6000_rtx_costs): Likewise.
> (rs6000_scalar_mode_supported_p): Likewise.
> (rs6000_update_ipa_fn_target_info): Likewise.
> (reg_to_non_prefixed): Likewise.
> (rs6000_split_logical_inner): Likewise.
> (rs6000_opaque_type_invalid_use_p): Likewise.
>
> ---
> gcc/config/rs6000/rs6000.cc | 495 ++--
> 1 file changed, 249 insertions(+), 246 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index d10d22a5816..43681d9eb08 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -5522,24 +5522,22 @@
> rs6000_cost_data::determine_suggested_unroll_factor (loop_vec_info
> loop_vinfo)
> to vectorize it with the unrolled VF any more if the actual
> iteration
> count is in between. */
> return 1;
> - else
> -{
> - unsigned int epil_niter_unr = est_niter % unrolled_vf;
> - unsigned int epil_niter = est_niter % vf;
> - /* Even if we have partial vector support, it can be still
> inefficent
> -to calculate the length when the iteration count is unknown, so
> -only expect it's good to unroll when the epilogue iteration count
> -is not bigger than VF (only one time length calculation). */
> - if (LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> - && epil_niter_unr <= vf)
> - return uf;
> - /* Without partial vector support, conservatively unroll this when
> -the epilogue iteration count is less than the original one
> -(epilogue execution time wouldn't be longer than before). */
> - else if (!LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> - && epil_niter_unr <= epil_niter)
> - return uf;
> -}
> +
> + unsigned int epil_niter_unr = est_niter % unrolled_vf;
> + unsigned int epil_niter = est_niter % vf;
> + /* Even if we have partial vector support, it can be still inefficent
> + to calculate the length when the iteration count is unknown, so
> + only expect it's good to unroll when the epilogue iteration count
> + is not bigger than VF (only one time length calculation). */
> + if (LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo) && epil_niter_unr
> <= vf)
> +return uf;
> +
> + /* Without partial vector support, conservatively unroll this when
> + the epilogue iteration count is less than the original one
> + (epilogue execution time wouldn't be longer than before). */
> + if (!LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> + && epil_niter_unr <= epil_niter)
> +return uf;
>
>return 1;
> }
> @@ -6044,35 +6042,31 @@ num_insns_constant_gpr (HOST_WIDE_INT value)
> return 1;
>
>/* constant loadable with addis */
> - else if ((value & 0x) == 0
> - && (value >> 31 == -1 || value >> 31 == 0))
> + if ((value & 0x) == 0 && (value >> 31 == -1 || value >> 31 == 0))
> return 1;
>
>/* PADDI can support up to 34 bit signed integers. */
> - else if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (value))
> + if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (value))
> return 1;
>
> - else if (TARGET_POWERPC64)
> -{
> - HOST_WIDE_INT low = sext_hwi (value, 32);
> - HOST_WIDE_INT high = value >> 31;
> + if (!TARGET_POWERPC64)
> +return 2;
>
> - if (high == 0 || high == -1)
> - return 2;
> + /* TARGET_POWERPC64 */
> + HOST_WIDE_INT low = sext_hwi (value, 32);
> + HOST_WIDE_INT high = value >> 31;
> +
> + if
[PATCH] early outs for functions in rs6000.cc
Hi,
There are some piece of code like below in rs6000.cc:
...
if (xx)
return x;
else if (yy)
return y;
... //else if chain
else
return d;
Using early outs would be more preferable for this kind of code.
The whole function rs6000_emit_set_long_const and num_insns_constant_gpr
are refined. And similar code are also restuctured in rs6000.cc.
This patch pass bootstrap and regtest on ppc64{,le}.
Is this ok for trunk?
BR,
Jeff (Jiufu Guo)
gcc/ChangeLog:
* config/rs6000/rs6000.cc (vspltis_shifted): Adopt early outs.
(rs6000_cost_data::determine_suggested_unroll_factor): Likewise.
(num_insns_constant_gpr): Likewise.
(easy_altivec_constant): Likewise.
(output_vec_const_move): Likewise.
(rs6000_expand_vector_set): Likewise.
(rs6000_legitimize_address): Likewise.
(rs6000_emit_set_long_const): Likewise.
(rs6000_preferred_reload_class): Likewise.
(rs6000_can_change_mode_class): Likewise.
(rs6000_output_move_128bit): Likewise.
(rs6000_emit_vector_cond_expr): Likewise.
(adjacent_mem_locations): Likewise.
(is_fusable_store): Likewise.
(insn_terminates_group_p): Likewise.
(rs6000_elf_reloc_rw_mask): Likewise.
(rs6000_rtx_costs): Likewise.
(rs6000_scalar_mode_supported_p): Likewise.
(rs6000_update_ipa_fn_target_info): Likewise.
(reg_to_non_prefixed): Likewise.
(rs6000_split_logical_inner): Likewise.
(rs6000_opaque_type_invalid_use_p): Likewise.
---
gcc/config/rs6000/rs6000.cc | 495 ++--
1 file changed, 249 insertions(+), 246 deletions(-)
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index d10d22a5816..43681d9eb08 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -5522,24 +5522,22 @@ rs6000_cost_data::determine_suggested_unroll_factor
(loop_vec_info loop_vinfo)
to vectorize it with the unrolled VF any more if the actual iteration
count is in between. */
return 1;
- else
-{
- unsigned int epil_niter_unr = est_niter % unrolled_vf;
- unsigned int epil_niter = est_niter % vf;
- /* Even if we have partial vector support, it can be still inefficent
-to calculate the length when the iteration count is unknown, so
-only expect it's good to unroll when the epilogue iteration count
-is not bigger than VF (only one time length calculation). */
- if (LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
- && epil_niter_unr <= vf)
- return uf;
- /* Without partial vector support, conservatively unroll this when
-the epilogue iteration count is less than the original one
-(epilogue execution time wouldn't be longer than before). */
- else if (!LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
- && epil_niter_unr <= epil_niter)
- return uf;
-}
+
+ unsigned int epil_niter_unr = est_niter % unrolled_vf;
+ unsigned int epil_niter = est_niter % vf;
+ /* Even if we have partial vector support, it can be still inefficent
+ to calculate the length when the iteration count is unknown, so
+ only expect it's good to unroll when the epilogue iteration count
+ is not bigger than VF (only one time length calculation). */
+ if (LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo) && epil_niter_unr <=
vf)
+return uf;
+
+ /* Without partial vector support, conservatively unroll this when
+ the epilogue iteration count is less than the original one
+ (epilogue execution time wouldn't be longer than before). */
+ if (!LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
+ && epil_niter_unr <= epil_niter)
+return uf;
return 1;
}
@@ -6044,35 +6042,31 @@ num_insns_constant_gpr (HOST_WIDE_INT value)
return 1;
/* constant loadable with addis */
- else if ((value & 0x) == 0
- && (value >> 31 == -1 || value >> 31 == 0))
+ if ((value & 0x) == 0 && (value >> 31 == -1 || value >> 31 == 0))
return 1;
/* PADDI can support up to 34 bit signed integers. */
- else if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (value))
+ if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (value))
return 1;
- else if (TARGET_POWERPC64)
-{
- HOST_WIDE_INT low = sext_hwi (value, 32);
- HOST_WIDE_INT high = value >> 31;
+ if (!TARGET_POWERPC64)
+return 2;
- if (high == 0 || high == -1)
- return 2;
+ /* TARGET_POWERPC64 */
+ HOST_WIDE_INT low = sext_hwi (value, 32);
+ HOST_WIDE_INT high = value >> 31;
+
+ if (high == 0 || high == -1)
+return 2;
- high >>= 1;
+ high >>= 1;
- if (low == 0 || low == high)
- return num_insns_constant_gpr (high) + 1;
- else if (high == 0)
- return num_insns_constant_gpr (low) + 1;
- else
- return (num_insns_constant_gpr (high)
- + num_insns_constant_gpr (low) + 1);
-}
