On Thu, Dec 5, 2024 at 2:13 PM Antoni Boucher <boua...@zoho.com> wrote:
>
> 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"?
Sometimes you want a 5bit integer type rather than say a 8bit one. Or
say you want one that is 1024bit integer type. Right now GCC JIT does
not have that concept at all but GCC's type system does support it
(via BITINT_TYPE for aarch64 and x86_64 targets). Well the GCC type
system has support <=64 bit types already even before BITINT_TYPE was
added.
Thanks,
Andrew
>
> >
> > Thanks,
> > Andrew
> >
> >
> >> Thanks for the review.
>
