Thanks for the review.
See explanations and questions below.
Le 2024-12-05 à 17 h 03, Andrew Pinski a écrit :
On Thu, Dec 5, 2024 at 1:46 PM Antoni Boucher <boua...@zoho.com> wrote:
Hi.
This is a patch for the bug 117886.
I ran the jit tests on x86-64 and there are as much failures as on the
master branch (4).
I also ran the jit tests on Aarch64 with another patch and there are
much less failures.
Iains ran the tests with this patch on Darwin and this fixes the
failures of a bunch of tests.
+ switch (size)
+ {
+ case 2:
+ /* FIXME: Wrong type. Update when Float16 is supported. */
This is not even close to being correct. since there are 2 short float
types for aarch64 (and x86 and arm). float16 and bf16.
More likely you should compare the real format of
GCC_JIT_TYPE_FLOAT/GCC_JIT_TYPE_DOUBLE to what the tree type. If that
is possible instead of the size. TYPE__PRECISION will get you similar
information as you do with `tree_to_uhwi (TYPE_SIZE_UNIT (type))`.
I get the feeling tree_type_to_jit_type is broken even more than that
though. Even doing `type == unsigned_intTI_type_node` is wrong.
I am suspecting for integral types you should handle precision form instead.
You don't have enough explanation of what is going wrong here in the
patch either and what is the fix either.
This is needed to better understand the design decisions on why
tree_type_to_jit_type is implemented this way. Because right now this
looks full on broken comparing different types to generate a JIT type
rather than having a way to create an arbitrary jit integral type (or
float type).
What is wrong is that we get types from target builtins that are not
supported by this function and as such, we get an ICE.
This fix add the support for the missing types.
libgccjit does type checking in the recording phase, where we don't have
access to tree types.
As such, the reason why whe have tree_type_to_jit_type is to be able to
type-check the calls of target builtin functions.
The current approach is far from ideal, but given that libgccjit is
structured this way (2 phases, recording and playback), the chosen
solution was to convert the tree types back to the jit types.
(This has the unintended consequence that we only have access to these
target builtins from the second compilation of libgccjit, which was
deemed acceptable, even thought I would prefer a better approach, since
this was used for other target-specific stuff like 128-bit integers
support.)
If you have any better idea, I'd be happy the change the current solution.
Is there any other information that you would need to better understand
the current design?
What do you mean by "arbitrary jit integral type"?
Thanks,
Andrew
Thanks for the review.
