Re: [PATCH] libgccjit: Fix float vector comparison

[Antoni Boucher <bouanto@zoho.com>]

On Thu, 2022-12-01 at 11:57 -0500, David Malcolm wrote:
> On Thu, 2022-12-01 at 10:33 -0500, Antoni Boucher wrote:
> > On Thu, 2022-12-01 at 10:25 -0500, David Malcolm wrote:
> > > On Thu, 2022-12-01 at 10:01 -0500, Antoni Boucher wrote:
> > > > Thanks, David.
> > > > Since we're not in phase 1 anymore, do we need an approval
> > > > before
> > > > I
> > > > merge like last year or can I merge immediately?
> > > 
> > > I think it counts as a bug fix and thus you can go ahead and
> > > merge
> > > (assuming you've done the usual testing).
> > > 
> > > > I also have many other patches (all in jit) that I need to
> > > > prepare
> > > > and
> > > > post to this mailing list.
> > > > What do you think?
> > > 
> > > Given that you're one of the main users of libgccjit I think
> > > there's
> > > a
> > > case for stretching the deadlines a bit here.
> > > 
> > > Do you have a repo I can look at?
> > 
> > Yes! The commits are in my fork:
> > https://github.com/antoyo/gcc
> > 
> > The only big one is the one adding support for target-dependent
> > builtins:
> > https://github.com/antoyo/gcc/commit/6d4313d4c02dd878f43917c978f299f5119330f0
> > 
> > Regarding this one, there's the issue that since we record the
> > builtins
> > on the first context run, we only have access to the builtins from
> > the
> > second run.
> > Do you have any idea how to fix this?
> > Or do you consider this is acceptable?
> 
> This is implemented behind the new
> gcc_jit_context_get_target_builtin_function entrypoint, right?

Yes.

> 
> If so, perhaps that recording::context::get_target_builtin_function
> could detect if it's the first time it's been called on this context,
> and if so make a playback::context to do the detection?  That way it
> would be transparent to the user, and work first time.

Oh, the issue is actually with the type reflection API and also the
type checking of function calls, so it's in the recording phase.
While I could think of a workaround for the type checking (e.g. delayed
type checking at the playback phase), I could not think of any better
solution for the type reflection.

> 
> 
> I see you have patches to add function and variable attributes; I
> wonder if this would be cleaner internally if there was a
> recording::attribute class, rather than the std::pair currently in
> use
> (some attributes have int arguments rather than string, others have
> multiple args).
> 
> I also wondered if a "gcc_jit_attribute" type could be exposed to the
> user, e.g.:
> 
>   attr1 = gcc_jit_context_new_attribute (ctxt, "noreturn");
>   attr2 = gcc_jit_context_new_attribute_with_string (ctxt, "alias",
> "__foo");
>   gcc_jit_function_add_attribute (ctxt, attr1);
>   gcc_jit_function_add_attribute (ctxt, attr2);
> 
> or somesuch?  But I think the API you currently have is OK.

Thanks for the suggestion; I'll look into that.

> 
> 
> > 
> > I also have a WIP branch which adds support for try/catch:
> > https://github.com/antoyo/gcc/commit/6219339fcacb079431596a0bc6cf8d430a1bd5a1
> > I'm not sure if this one is going to be ready soon or not.
> 
> I see that the new entrypoints have e.g.:
> 
> /* Add a try/catch statement.
>    This is equivalent to this C++ code:
>      try {
>         try_block
>      }
>      catch {
>         catch_block
>      }
> */
> 
> void
> gcc_jit_block_add_try_catch (gcc_jit_block *block,
>                              gcc_jit_location *loc,
>                              gcc_jit_block *try_block,
>                              gcc_jit_block *catch_block);
> 
> but I'm not sure how this is meant to interact with the CFG-like
> model
> used by the rest of the gcc_jit_block_* API.  What happens at the end
> of the blocks?  Does the generated code use the C++ ABI for
> exception-
> handling?

Currently, it requires the try and catch blocks to be terminated, but
also require the block containing the try/catch to be terminated.
That doesn't make sense.
Would it be OK if it doesn't require the try and catch blocks to be
terminated?

For the ABI, I'm not sure it's necessarily tied to C++, but I might be
wrong. From what I understand, GCC will use the dwarf-2 exception
handling model if it's available or the sjlj otherwise (perhaps that
can be configured).
And the user can change the personality function via the API I added.

Thanks for your feedback.

> 
> Dave
> 
> > 
> > Thanks.
> > 
> > > 
> > > Dave
> > > 
> > > 
> > > > 
> > > > On Thu, 2022-12-01 at 09:28 -0500, David Malcolm wrote:
> > > > > On Sun, 2022-11-20 at 14:03 -0500, Antoni Boucher via Jit
> > > > > wrote:
> > > > > > Hi.
> > > > > > This fixes bug 107770.
> > > > > > Thanks for the review.
> > > > > 
> > > > > Thanks, the patch looks good to me.
> > > > > 
> > > > > Dave
> > > > > 
> > > > 
> > > 
> > 
>