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

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2023-11-20 at 23:10 +0100, Petter Tomner wrote:
> This patch fixes support for complex types.
> 
> It adds the entrypoints:
> gcc_jit_context_new_rvalue_from_complex_double
> gcc_jit_context_set_bool_enable_complex_types
> 
> Aswell as the binary operator GCC_JIT_BINARY_OP_COMPLEX, to make
> complex numbers from two real number rvalues.
> 
> Note that the complex types already are in the type enum, so I
> thought the
> cleanest solution is to enable them, rather than to have a
> "get_complex_type"
> function.
> 
> When I wrote the patch originally, the complex enums were not usable
> due to a
> range check, but I think they accidentally were included when int8_t
> etc were added.
> However, casts to complex types segfaults currently and some other
> issues, so I would say
> an explicit enable is the least confusing, since the enums have been
> there since the get go.
> 
> See attachement.

> Subject: [PATCH 1/2] Complex
> 
> Fix support for complex types

Thanks for the patch.

> 
> The patch adds support of complex floating point types to libgccjit.
> A new binary operator 'COMPLEX' is added to create complex values
> from real values. Aswell as a function to create a complex double
> literal.

The patch adds usage of "_Complex" to libgccjit.h.  According to
https://en.cppreference.com/w/c/language/arithmetic_types#Complex_floating_types
this is a C99 feature.

I think libgccjit.h is currently only requiring C89 (but I'm not sure),
so this declaration probably needs some kind of "C99 or later"
preprocessor guard.

Also, this needs to be includeable from C++; I'm not sure what the
precise requirements there are.

> 
> To notify users if a binary linking to libgccjit depends on complex
> types, complex type support most be enabled with a new option function
> since they allready are in the types enum.

I like this approach, but I see that the patch only uses it to guard
gcc_jit_context_get_type.

It seems to me that it should guard every entrypoint that uses an enum
that's expanded by the API, so presumably it should also guard
gcc_jit_context_new_binary_op with op == GCC_JIT_BINARY_OP_COMPLEX. 
Although you'd need a complex type from gcc_jit_context_get_type to get
the result, it's probably easier for the user to understand if we
document and validate the gcc_jit_context_new_binary_op with op ==
GCC_JIT_BINARY_OP_COMPLEX explicitly.

I looked through the rest of the patch and it seems OK, though I'm
nervous about all the tests that are comparing floating point values
for equality; do we always get the precise expected value, on every
target?  Floating point equality comparisons tend to be a nightmare in
unit tests.

Thanks again for the patch
Dave