Hi Aaron,
On Mon, Feb 04, 2019 at 01:06:57PM -0600, Aaron Sawdey wrote:
> This is the second part of the fix for 89112, fixing the conditions that
> caused it to happen.
> This patch adds REG_BR_PROB notes to the branches generated by inline
> expansion of memcmp
> and strncmp. This prevents any of the code from being marked as cold and
> moved to the end
> of the function, which is what caused the long branches in 89112. With this
> patch, the test
> case for 89112 does not have any long branches within the expansion of
> memcmp, and the code
> for each memcmp is contiguous.
I think it is a good idea to have this in 9. But, a few things need
fixing:
> 2019-02-04 Aaron Sawdey <acsaw...@linux.ibm.com>
>
> * config/rs6000/rs6000-string.c (do_ifelse, expand_cmp_vec_sequence,
> expand_compare_loop, expand_block_compare_gpr,
> expand_strncmp_align_check, expand_strncmp_gpr_sequence): add branch
> probability.
"Add", capital. Maybe write it different, it sounds like addition of
probabilities to me :-/
> Index: gcc/config/rs6000/rs6000-string.c
> ===================================================================
> --- gcc/config/rs6000/rs6000-string.c (revision 268522)
> +++ gcc/config/rs6000/rs6000-string.c (working copy)
> @@ -35,6 +35,8 @@
> #include "expr.h"
> #include "output.h"
> #include "target.h"
> +#include "profile-count.h"
> +#include "predict.h"
These should go into the changelog. The changelog can use a few more words
in general.
> static void
> do_ifelse (machine_mode cmpmode, rtx_code comparison,
> - rtx a, rtx b, rtx cr, rtx true_label)
> + rtx a, rtx b, rtx cr, rtx true_label, profile_probability p)
Please call it br_prob or such, not just "p".
> @@ -1120,11 +1125,11 @@
> /* Check for > max_bytes bytes. We want to bail out as quickly as
> possible if we have to go over to memcmp. */
> do_ifelse (CCmode, GT, bytes_rtx, GEN_INT (max_bytes),
> - NULL_RTX, library_call_label);
> + NULL_RTX, library_call_label, profile_probability::even ());
>
> /* Check for < loop_bytes bytes. */
> do_ifelse (CCmode, LT, bytes_rtx, GEN_INT (loop_bytes),
> - NULL_RTX, cleanup_label);
> + NULL_RTX, cleanup_label, profile_probability::even ());
Is "even" a good guess here? Is there no preferred direction? "likely"
and "unlikely" are only 80% and 20% as well.
> @@ -1165,7 +1170,7 @@
> {
> rtx lab_after = gen_label_rtx ();
> do_ifelse (CCmode, LE, bytes_rtx, GEN_INT (max_bytes),
> - NULL_RTX, lab_after);
> + NULL_RTX, lab_after, profile_probability::even ());
Same here, wherever that is :-)
> @@ -1363,10 +1374,12 @@
> if (bytes_is_const)
> bytes_remaining -= load_mode_size;
> else
> - /* See if remaining length is now zero. We previously set
> - target to 0 so we can just jump to the end. */
> - do_ifelse (CCmode, EQ, cmp_rem, const0_rtx,
> - NULL_RTX, final_label);
> + {
> + /* See if remaining length is now zero. We previously set
> + target to 0 so we can just jump to the end. */
> + do_ifelse (CCmode, EQ, cmp_rem, const0_rtx, NULL_RTX,
> + final_label, profile_probability::unlikely ());
> + }
Why the extra braces? Sometimes that helps, but it's only clutter here.
Okay for trunk and backports with those things fixed (or post it again, if
you prefer). Thanks!
Segher
