Re: [PATCH 2/2] jit: Complex types and loong constants

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2023-11-20 at 23:15 +0100, Petter Tomner wrote:
> This patch adds the possibility to make the following constants:
> * long long
> * long double
> * complex long double
> 
> The long long one is needed for 32bit systems to make 64bit
> literals without union workarounds.
> 
> The new entrypoints are:
> gcc_jit_context_new_rvalue_from_long_long
> gcc_jit_context_new_rvalue_from_long_double
> gcc_jit_context_new_rvalue_from_complex_long_double
> 
> The patch also fixes a issue with the reproducer's debug
> c-file writer, which does not handle floating point numbers
> very well. I.e. infs, NaN and losing precision on doubles.
> 
> make check-jit runs fine with the patch series on 64bit Debian
> as well as on 32bit Debian in a VM, with a GNU/Linux setup.
> 

Thanks for this patch

[...]

> diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
> index 6922a2f67e9..e8b16926330 100644
> --- a/gcc/jit/docs/topics/compatibility.rst
> +++ b/gcc/jit/docs/topics/compatibility.rst
> @@ -384,13 +384,15 @@ alignment of a variable:
>  ``LIBGCCJIT_ABI_26``
>  --------------------
>  ``LIBGCCJIT_ABI_26`` covers the addition of support for complex types,
> -literals for complex types as well as the
> +literals for long long, long double and complex types as well as the
>  complex binary operator.

You're extending the definition of the numbered ABI here.  I'm OK with
that if the 2nd patch goes in immediately after the 1st.


[...]

>  template <>
> diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
> index ed788502c15..baa75311b31 100644
> --- a/gcc/jit/jit-recording.cc
> +++ b/gcc/jit/jit-recording.cc
> @@ -26,6 +26,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "toplev.h"
>  
>  #include <complex.h>
> +#include <float.h>
>  
>  #include "jit-builtins.h"
>  #include "jit-recording.h"
> @@ -1836,8 +1837,48 @@ recording::context::dump_reproducer_to_file (const char *path)
>  	   " gcc_jit_context_dump_reproducer_to_file.\n\n");
>    print_version (r.get_file (), "  ", false);
>    r.write ("*/\n");
> -  r.write ("#include <libgccjit.h>\n\n");
> -  r.write ("#pragma GCC diagnostic ignored \"-Wunused-variable\"\n\n");
> +  r.write (
> +    "#include <libgccjit.h>\n"
> +    "#include <math.h>\n"
> +    "#include <complex.h>\n"
> +    "#include <string.h>\n\n");
> +  r.write ("#pragma GCC diagnostic ignored \"-Wunused-variable\"\n");
> +  r.write ("#pragma GCC diagnostic ignored \"-Wunused-function\"\n\n");
> +  /* Type punning unions */
> +  r.write ("union dull { double d; unsigned long long ull; };\n\n");
> +  r.write (
> +    "union ldarr { long double ld; unsigned arr[sizeof (long double)];};\n\n");
> +
> +  /* Functions for type punning nan:s, keeping bit representation */
> +  r.write (
> +    "/* Convert type punned uint 64 to double NaN.\n"
> +    "   Might lose/corrupt payload between architectures. */\n"
> +    "static double\n"
> +    "ull_to_d (unsigned long long ull)\n"
> +    "{\n"
> +    "  union dull u; u.ull = ull;\n"
> +    "  /* Paranoia check for foreign NaN representation. */\n"
> +    "  if (!isnan (u.d)) return NAN;\n"
> +    "  return u.d;\n"
> +    "}\n\n");
> +  r.write (
> +    "/* Convert array to long double NaN.\n"
> +    "   Might lose/corrupt payload between architectures. */\n"
> +    "static long double\n"
> +    "arr_to_ld (unsigned char *arr, int len)\n"
> +    "{\n"
> +    "  union ldarr u;\n"
> +    /* Since long double sizes can vary between architectures,
> +       we need to handle that. */
> +    "  /* For foreign long double sizes */\n"
> +    "  if (sizeof u.arr != len) return NAN;\n"
> +    "  memcpy (u.arr, arr, len);\n"
> +    /* The size might fool us becouse of padding, so check it is a nan. */
> +    "  /* Handle foreign representations that is not NaN on this machine. */\n"
> +    "  if (!isnan (u.ld)) return NAN;\n"
> +    "  return u.ld;\n"
> +    "}\n\n");

There are various places above that make the reproducer rquire C99/C++11:
- <complex.h> is C99 onwards
- "long long" is C99 onwards
- "isnan" and "NAN" are C99 and C++11.

Perhaps we could add preprocessor guards here.  But maybe the things
that need these are only expressable in code that was written in that
version of the language, so maybe only write out these things if
they're needed?  That might eliminate a lot of preprocessor gunk in the
generated reproducer, at the cost of some complexity in the
dump_reproducer logic.

Or we could simply require the user to use C99/C++11 for generated
reproducers.  That's probably simplest.

[...]

> +/* For typepruning NaNs. */
          ^^^^^^^^^^^

Is this a typo for "typepunning"...

> +union dull {
> +  double d;
> +  unsigned long long ull;
> +};
> +
> +union ldarr {
> +  long double ld;
> +  unsigned char arr[sizeof(long double)];
> +};
> +
> +

...here?

[...]

> +/* Helper function for write_reproducer()
> +
> +   Write an array of unsigned chars to the reproducer, like:
> +   {0x12, 0x34, ... } */
> +static void
> +reproducer_write_arr (reproducer &r, unsigned char *arr, int len)

"arr" should probably be const, and "len" be a size_t.

> +{
> +  if (len == 0)
> +    return;
> +
> +  r.write ("{");
> +  for (int i = 0; i < len - 1; i++)
> +    r.write ("%#x , ", arr[i]);
> +  r.write ("%#x }", arr[len - 1]);
> +}

[...]

> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> index cdaec450981..2134c92465e 100644
> --- a/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h
> @@ -1069,6 +1069,16 @@ gcc_jit_context_new_rvalue_from_long (gcc_jit_context *ctxt,
>  				      gcc_jit_type *numeric_type,
>  				      long value);
>  
> +#define LIBGCCJIT_HAVE_LONGLONG_CONSTANTS
> +/* Get a constant from a long long.
> +
> +   This function was added in LIBGCCJIT_ABI_26. You can test for
> +   its presence with:
> +     #ifdef LIBGCCJIT_HAVE_LONGLONG_CONSTANTS */
> +extern gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
> +					   gcc_jit_type *numeric_type,
> +					   long long value);

"long long" is C99 / C++11; can we add a suitable preprocessor guard
for this declaration?


[...]

> +/* Get a constant from a long double.
> +
> +   This function was added in LIBGCCJIT_ABI_26. You can test for
> +   its presence with:
> +     #ifdef LIBGCCJIT_HAVE_LONGLONG_CONSTANTS */
> +extern gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_long_double (gcc_jit_context *ctxt,
> +					     gcc_jit_type *numeric_type,
> +					     long double value);

FWIW "LIBGCCJIT_HAVE_LONGLONG_CONSTANTS" doesn't seem quite like the
right name here, but I wasn't able to come up with a better one.

> +/* Get a constant from a complex double.
>  
>     This API entrypoint was added in LIBGCCJIT_ABI_26; you can test for its
>     presence using
> @@ -1093,6 +1113,17 @@ gcc_jit_context_new_rvalue_from_complex_double (gcc_jit_context *ctxt,
>  						gcc_jit_type *numeric_type,
>  						_Complex double value);
>  
> +/* Get a constant from a complex long double.
> +
> +   This API entrypoint was added in LIBGCCJIT_ABI_26; you can test for its
> +   presence using
> +     #ifdef LIBGCCJIT_HAVE_LONGLONG_CONSTANTS */

Likewise here.

> +extern gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_complex_long_double (
> +  gcc_jit_context *ctxt,
> +  gcc_jit_type *numeric_type,
> +  _Complex long double value);
> +

As noted in the review of the other patch "_Complex", is C99 (and I
don't know which C++ version), so some kind of preprocessor guard would
be good here, so that we don't impose a requirement on all users of
libgccjit.h

[...]

As before, I looked through the rest of the patch and it seems OK.

Thanks again for the patch
Dave