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