Re: [PATCH] [i386] Support type _Float16/__bf16 independent of SSE2.

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Apr 19, 2023 at 03:15:51PM +0800, liuhongt wrote:
ChangeLog nits have been already reported earlier.

> --- a/gcc/config/i386/i386-c.cc
> +++ b/gcc/config/i386/i386-c.cc
> @@ -817,6 +817,43 @@ ix86_target_macros (void)
>    if (!TARGET_80387)
>      cpp_define (parse_in, "_SOFT_FLOAT");
>  
> +  /* HFmode/BFmode is supported without depending any isa
> +     in scalar_mode_supported_p and libgcc_floating_mode_supported_p,
> +     but according to psABI, they're really supported w/ SSE2 and above.
> +     Since libstdc++ uses __STDCPP_FLOAT16_T__ and __STDCPP_BFLOAT16_T__
> +     for backend support of the types, undef the macros to avoid
> +     build failure, see PR109504.  */
> +  if (!TARGET_SSE2)
> +    {
> +      if (c_dialect_cxx ()
> +	  && cxx_dialect > cxx20)

Formatting, both conditions are short, so just put them on one line.

> +	{
> +	  cpp_undef (parse_in, "__STDCPP_FLOAT16_T__");
> +	  cpp_undef (parse_in, "__STDCPP_BFLOAT16_T__");
> +	}

But for the C++23 macros, more importantly I think we really should
also in ix86_target_macros_internal add
  if (c_dialect_cxx ()
      && cxx_dialect > cxx20
      && (isa_flag & OPTION_MASK_ISA_SSE2))
    {
      def_or_undef (parse_in, "__STDCPP_FLOAT16_T__");
      def_or_undef (parse_in, "__STDCPP_BFLOAT16_T__");
    }
plus associated libstdc++ changes.  It can be done incrementally though.

> +
> +      if (flag_building_libgcc)
> +	{
> +	  /* libbid uses __LIBGCC_HAS_HF_MODE__ and __LIBGCC_HAS_BF_MODE__
> +	     to check backend support of _Float16 and __bf16 type.  */

That is actually the case only for HFmode, but not for BFmode right now.
So, we need further work.  One is to add the BFmode support in there,
and another one is make sure the _Float16 <-> _Decimal* and __bf16 <->
_Decimal* conversions are compiled in also if not -msse2 by default.
One way to do that is wrap the HF and BF mode related functions on x86
#ifndef __SSE2__ into the pragmas like intrin headers use (but then
perhaps we don't need to undef this stuff here), another is not provide
the hf/bf support in that case from the TUs where they are provided now,
but from a different one which would be compiled with -msse2.

> +	  cpp_undef (parse_in, "__LIBGCC_HAS_HF_MODE__");
> +	  cpp_undef (parse_in, "__LIBGCC_HF_FUNC_EXT__");
> +	  cpp_undef (parse_in, "__LIBGCC_HF_MANT_DIG__");
> +	  cpp_undef (parse_in, "__LIBGCC_HF_EXCESS_PRECISION__");
> +	  cpp_undef (parse_in, "__LIBGCC_HF_EPSILON__");
> +	  cpp_undef (parse_in, "__LIBGCC_HF_MAX__");
> +	  cpp_undef (parse_in, "__LIBGCC_HF_MIN__");
> +
> +	  cpp_undef (parse_in, "__LIBGCC_HAS_BF_MODE__");
> +	  cpp_undef (parse_in, "__LIBGCC_BF_FUNC_EXT__");
> +	  cpp_undef (parse_in, "__LIBGCC_BF_MANT_DIG__");
> +	  cpp_undef (parse_in, "__LIBGCC_BF_EXCESS_PRECISION__");
> +	  cpp_undef (parse_in, "__LIBGCC_BF_EPSILON__");
> +	  cpp_undef (parse_in, "__LIBGCC_BF_MAX__");
> +	  cpp_undef (parse_in, "__LIBGCC_BF_MIN__");
> +	}
> +    }
> +

> --- a/gcc/config/i386/i386.cc
> +++ b/gcc/config/i386/i386.cc
> @@ -2651,7 +2651,10 @@ construct_container (machine_mode mode, machine_mode orig_mode,
>  
>    /* We allowed the user to turn off SSE for kernel mode.  Don't crash if
>       some less clueful developer tries to use floating-point anyway.  */
> -  if (needed_sseregs && !TARGET_SSE)
> +  if (needed_sseregs
> +      && (!TARGET_SSE
> +	  || (VALID_SSE2_TYPE_MODE (mode)
> +	      && !TARGET_SSE2)))

Formatting, no need to split this up that much.
  if (needed_sseregs
      && (!TARGET_SSE
	  || (VALID_SSE2_TYPE_MODE (mode) && !TARGET_SSE2)))
or even better
  if (needed_sseregs
      && (!TARGET_SSE || (VALID_SSE2_TYPE_MODE (mode) && !TARGET_SSE2)))
will do it.

> @@ -22805,9 +22827,10 @@ ix86_emit_support_tinfos (emit_support_tinfos_callback callback)
>  
>    if (!TARGET_SSE2)
>      {
> -      gcc_checking_assert (!float16_type_node && !bfloat16_type_node);
> -      float16_type_node = ix86_float16_type_node;
> -      bfloat16_type_node = ix86_bf16_type_node;
> +      float16_type_node
> +	= float16_type_node ? float16_type_node : ix86_float16_type_node;
> +      bfloat16_type_node
> +	= bfloat16_type_node ? bfloat16_type_node : ix86_bf16_type_node;
>        callback (float16_type_node);
>        callback (bfloat16_type_node);

Instead of this, just use
      if (!float16_type_node)
	{
	  float16_type_node = ix86_float16_type_node;
	  callback (float16_type_node);
	  float16_type_node = NULL_TREE;
	}
      if (!bfloat16_type_node)
	{
	  bfloat16_type_node = ix86_bf16_type_node;
	  callback (bfloat16_type_node);
	  bfloat16_type_node = NULL_TREE;
	}
?
> +/* Return the diagnostic message string if conversion from FROMTYPE to
> +   TOTYPE is not allowed, NULL otherwise.  */
> +
> +static const char *
> +ix86_invalid_conversion (const_tree fromtype, const_tree totype)
> +{
> +  if (element_mode (fromtype) != element_mode (totype))
> +    {
> +      /* Do no allow conversions to/from BFmode/HFmode scalar types
> +	 when TARGET_SSE2 is not available.  */
> +      if ((TYPE_MODE (fromtype) == BFmode
> +	   || TYPE_MODE (fromtype) == HFmode)
> +	  && !TARGET_SSE2)

First of all, not really sure if this should be purely about scalar
modes, not also complex and vector modes involving those inner modes.
Because complex or vector modes with BF/HF elements will be without
TARGET_SSE2 for sure lowered into scalar code and that can't be handled
either.
So if (!TARGET_SSE2 && GET_MODE_INNER (TYPE_MODE (fromtype)) == BFmode)
or even better
if (!TARGET_SSE2 && element_mode (fromtype) == BFmode)
?
Or even better remember the 2 modes above into machine_mode temporaries
and just use those in the != comparison and for the checks?

Also, I think it is weird to tell user %<__bf16%> or %<_Float16%> when
we know which one it is.  Just return separate messages?


> +	return N_("invalid conversion from type %<__bf16%> "
> +		  "or %<_Float16%> without option %<-msse2%>");
> +
> +      if ((TYPE_MODE (totype) == BFmode
> +	   || TYPE_MODE (totype) == HFmode)
> +	  && !TARGET_SSE2)
> +	return N_("invalid conversion to type %<__bf16%> "
> +		  "or %<_Float16%> without option %<-msse2%>");

Ditto.
> +    }
> +
> +  /* Conversion allowed.  */
> +  return NULL;
> +}
> +
> +/* Return the diagnostic message string if the unary operation OP is
> +   not permitted on TYPE, NULL otherwise.  */
> +
> +static const char *
> +ix86_invalid_unary_op (int op, const_tree type)
> +{
> +  /* Reject all single-operand operations on BFmode/HFmode except for &
> +     when TARGET_SSE2 is not available.  */
> +  if ((element_mode (type) == BFmode || element_mode (type) == HFmode)
> +      && !TARGET_SSE2 && op != ADDR_EXPR)
> +    return N_("operation not permitted on type %<__bf16%> "
> +	      "or %<_Float16%> without option %<-msse2%>");

Similarly.  Also, check !TARGET_SSE2 first as inexpensive one.
> +
> +  /* Operation allowed.  */
> +  return NULL;
> +}
> +
> +/* Return the diagnostic message string if the binary operation OP is
> +   not permitted on TYPE1 and TYPE2, NULL otherwise.  */
> +
> +static const char *
> +ix86_invalid_binary_op (int op ATTRIBUTE_UNUSED, const_tree type1,
> +			   const_tree type2)
> +{
> +  /* Reject all 2-operand operations on BFmode or HFmode
> +     when TARGET_SSE2 is not available.  */
> +  if ((element_mode (type1) == BFmode
> +       || element_mode (type2) == BFmode
> +       || element_mode (type1) == HFmode
> +       || element_mode (type2) == HFmode)
> +      && !TARGET_SSE2)
> +    return N_("operation not permitted on type %<__bf16%> "
> +	      "or %<_Float16%> without option %<-msse2%>");

Similarly.

	Jakub