Hongyu Wang <hongyu.w...@intel.com> writes:
> Hi,
>
> In cfgexpand, there is an optimization for branch which tests
> targetm.gen_ccmp_first == NULL. However for target like x86-64, the
> hook was implemented but it does not indicate that ccmp was enabled.
> Add a new target hook TARGET_HAVE_CCMP and replace the middle-end
> check for the existance of gen_ccmp_first to avoid misoptimization.
>
> This fixes PR115370 that have suboptimal codegen, also I checked the
> znver2 binary for 526 and it will have same binary as the one before
> the CCMP support patch r15-1058, so suppose it could also fix PR115463.
It's unfortunate that we can't simply try expanding ccmp and seeing
whether it works, but I agree that that isn't possible for the
cfgexpand.cc change.
The expr.cc change shouldn't be needed, but I agree it's more consistent
to change both places in the same way.
> Bootstrapped/regtested on x86-64-pc-linux-gnu and aarch64-none-linux-gnu.
>
> Ok for trunk?
>
> gcc/ChangeLog:
>
> PR target/115370
> PR target/115463
> * cfgexpand.cc (expand_gimple_cond): Call targetm.have_ccmp
> instead of checking if targetm.gen_ccmp_first exists.
> * expr.cc (expand_expr_real_gassign): Likewise.
> * config/i386/i386.cc (ix86_have_ccmp): New target hook to
> check if APX_CCMP enabled.
> (TARGET_HAVE_CCMP): Define.
> * doc/tm.texi: Add TARGET_HAVE_CCMP.
> * doc/tm.texi.in: Regenerated.
> * target.def (TARGET_HAVE_CCMP): New target hook.
> * targhooks.cc (default_have_ccmp): New function.
> * targhooks.h (default_have_ccmp): New prototype.
> ---
> gcc/cfgexpand.cc | 2 +-
> gcc/config/i386/i386.cc | 9 +++++++++
> gcc/doc/tm.texi | 6 ++++++
> gcc/doc/tm.texi.in | 2 ++
> gcc/expr.cc | 2 +-
> gcc/target.def | 9 +++++++++
> gcc/targhooks.cc | 6 ++++++
> gcc/targhooks.h | 1 +
> 8 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
> index 8de5f2ba58b..dad3ae1b7c6 100644
> --- a/gcc/cfgexpand.cc
> +++ b/gcc/cfgexpand.cc
> @@ -2646,7 +2646,7 @@ expand_gimple_cond (basic_block bb, gcond *stmt)
> /* If jumps are cheap and the target does not support conditional
> compare, turn some more codes into jumpy sequences. */
> else if (BRANCH_COST (optimize_insn_for_speed_p (), false) < 4
> - && targetm.gen_ccmp_first == NULL)
> + && !targetm.have_ccmp ())
> {
> if ((code2 == BIT_AND_EXPR
> && TYPE_PRECISION (TREE_TYPE (op0)) == 1
> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> index 173db213d14..c72f64da983 100644
> --- a/gcc/config/i386/i386.cc
> +++ b/gcc/config/i386/i386.cc
> @@ -26204,6 +26204,13 @@ ix86_memtag_add_tag (rtx base, poly_int64 offset,
> unsigned char tag_offset)
> return plus_constant (Pmode, tagged_addr, offset);
> }
>
> +/* Implement TARGET_HAVE_CCMP. */
> +static bool
> +ix86_have_ccmp ()
> +{
> + return (bool) TARGET_APX_CCMP;
> +}
> +
> /* Target-specific selftests. */
>
> #if CHECKING_P
> @@ -27043,6 +27050,8 @@ ix86_libgcc_floating_mode_supported_p
> #undef TARGET_GEN_CCMP_NEXT
> #define TARGET_GEN_CCMP_NEXT ix86_gen_ccmp_next
>
> +#undef TARGET_HAVE_CCMP
> +#define TARGET_HAVE_CCMP ix86_have_ccmp
>
> static bool
> ix86_libc_has_fast_function (int fcode ATTRIBUTE_UNUSED)
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index 8a7aa70d605..993816deeba 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -12354,6 +12354,12 @@ This function prepares to emit a conditional
> comparison within a sequence
> @var{bit_code} is @code{AND} or @code{IOR}, which is the op on the compares.
> @end deftypefn
>
> +@deftypefn {Target Hook} bool TARGET_HAVE_CCMP (void)
> +This target hook returns true if the target supports conditional compare.
> +This target hook is required only when the target has conditional compare
> that
> +was not enabled by default, such as x86-64.
Maybe the second sentence could be something like:
-----
The hook is required only when the support is conditionally enabled,
such as in response to command-line flags. The default implementation
returns true iff @code{TARGET_GEN_CCMP_FIRST} is defined.
-----
The reason for the suggestion is that "was enabled by default" sounds to
me like it's describing behaviour that can be altered by command-line flags,
rather than something that happens unconditionally.
OK with that change, thanks.
Richard
> +@end deftypefn
> +
> @deftypefn {Target Hook} unsigned TARGET_LOOP_UNROLL_ADJUST (unsigned
> @var{nunroll}, class loop *@var{loop})
> This target hook returns a new value for the number of times @var{loop}
> should be unrolled. The parameter @var{nunroll} is the number of times
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 9e0830758ae..87a7f895174 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -7923,6 +7923,8 @@ lists.
>
> @hook TARGET_GEN_CCMP_NEXT
>
> +@hook TARGET_HAVE_CCMP
> +
> @hook TARGET_LOOP_UNROLL_ADJUST
>
> @defmac POWI_MAX_MULTS
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 1baa39b98eb..04bad5e1425 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -11089,7 +11089,7 @@ expand_expr_real_gassign (gassign *g, rtx target,
> machine_mode tmode,
> ops.op1 = gimple_assign_rhs2 (g);
>
> /* Try to expand conditonal compare. */
> - if (targetm.gen_ccmp_first)
> + if (targetm.have_ccmp ())
> {
> gcc_checking_assert (targetm.gen_ccmp_next != NULL);
> r = expand_ccmp_expr (g, TYPE_MODE (ops.type));
> diff --git a/gcc/target.def b/gcc/target.def
> index 70070caebc7..1511038785d 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -2783,6 +2783,15 @@ DEFHOOK
> rtx, (rtx_insn **prep_seq, rtx_insn **gen_seq, rtx prev, rtx_code cmp_code,
> tree op0, tree op1, rtx_code bit_code),
> NULL)
>
> +/* Return true if the target supports conditional compare. */
> +DEFHOOK
> +(have_ccmp,
> + "This target hook returns true if the target supports conditional
> compare.\n\
> +This target hook is required only when the target has conditional compare
> that\n\
> +was not enabled by default, such as x86-64.",
> + bool, (void),
> + default_have_ccmp)
> +
> /* Return a new value for loop unroll size. */
> DEFHOOK
> (loop_unroll_adjust,
> diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
> index fb339bf75dd..4f53257e55c 100644
> --- a/gcc/targhooks.cc
> +++ b/gcc/targhooks.cc
> @@ -1887,6 +1887,12 @@ default_have_conditional_execution (void)
> return HAVE_conditional_execution;
> }
>
> +bool
> +default_have_ccmp (void)
> +{
> + return targetm.gen_ccmp_first != NULL;
> +}
> +
> /* By default we assume that c99 functions are present at the runtime,
> but sincos is not. */
> bool
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index 85f3817c176..f53913ebdfa 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -216,6 +216,7 @@ extern void default_addr_space_diagnose_usage
> (addr_space_t, location_t);
> extern rtx default_addr_space_convert (rtx, tree, tree);
> extern unsigned int default_case_values_threshold (void);
> extern bool default_have_conditional_execution (void);
> +extern bool default_have_ccmp (void);
>
> extern bool default_libc_has_function (enum function_class, tree);
> extern bool default_libc_has_fast_function (int fcode);
