On Wed, Sep 02, 2020 at 09:57:08AM +0800, Hongtao Liu via Gcc-patches wrote:
> +
> +      first = XVECEXP (constant, 0, 0);
> +      /* There could be some rtx like
> +      (mem/u/c:V16QI (symbol_ref/u:DI ("*.LC1")))
> +      but with "*.LC1" refer to V2DI constant vector.  */
> +      if (GET_MODE (constant) != mode)
> +     {
> +       constant = simplify_subreg (mode, constant, GET_MODE (constant), 0);
> +       if (constant == NULL_RTX || GET_CODE (constant) != CONST_VECTOR)
> +         return;
> +     }

The
      first = XVECEXP (constant, 0, 0);
line needs to be after this if, not before it, otherwise it will miscompile
things or just ICE.

> @@ -2197,6 +2272,10 @@ remove_partial_avx_dependency (void)
>         if (!NONDEBUG_INSN_P (insn))
>           continue;
>  
> +       /* Hanlde AVX512 embedded broadcast here to save compile time.  */

s/Hanlde/Handle/

> +  for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
> +    {
> +      if (!INSN_P (insn))
> +     continue;
> +      replace_constant_pool_with_broadcast (insn);
> +    }

Perhaps instead do:
  for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
    if (INSN_P (insn))
      replace_constant_pool_with_broadcast (insn);
?

> +  /* opt_pass methods: */
> +  virtual bool gate (function *)
> +    {
> +      /* Return false if rpad pass gate is true.
> +      replace_constant_pool_with_broadcast is called
> +      from both this pass and rpad pass.  */
> +      return (TARGET_AVX512F
> +           && !(TARGET_AVX
> +                && TARGET_SSE_PARTIAL_REG_DEPENDENCY
> +                && TARGET_SSE_MATH
> +                && optimize
> +                && optimize_function_for_speed_p (cfun)));

I think this could be a maintainance nightmare.
Perhaps instead add

static bool
remove_partial_avx_dependency_gate ()
{
  return (TARGET_AVX
          && TARGET_SSE_PARTIAL_REG_DEPENDENCY
          && TARGET_SSE_MATH
          && optimize
          && optimize_function_for_speed_p (cfun));
}
after the remove_partial_avx_dependency function definition,
change pass_remove_partial_avx_dependency gate body to
      return remove_partial_avx_dependency_gate ();
and in pass_constant_pool_broadcast::gate do
      return (TARGET_AVX512F && !remove_partial_avx_dependency_gate ();
(with the comment you have there)?

LGTM with those changes.

        Jakub

Reply via email to