From: Basile Starynkevitch <basile@starynkevitch.net>
To: David Malcolm <dmalcolm@redhat.com>
Cc: jit@gcc.gnu.org
Subject: Re: PATCH (v2) trunk GCCJIT: adding gcc_jit_context_new_rvalue_from_long_long, etc...
Date: Thu, 01 Jan 2015 00:00:00 -0000 [thread overview]
Message-ID: <55A93BAE.7060403@starynkevitch.net> (raw)
In-Reply-To: <1437140458.15571.58.camel@surprise>
On 07/17/2015 15:40, David Malcolm wrote:
>
>
> I'd prefer to just have:
>
> gcc_jit_context_new_rvalue_from_long_long
> gcc_jit_context_new_rvalue_from_unsigned_long_long
Ok, working on that. I'm not CC-ing gcc-patches@ because I have only
comments here... Will submit my v3 patch after you've had explained me
things...
>
> and not bother with the int32_t, int64_t, intptr_t.
>
> I think if we're adding "long long", we need "unsigned long long", given
> that there are values of the latter that aren't yet expressible without
> resorting to hacks.
Ok. working on that.
> Note that in:
> gcc_jit_context_new_rvalue_from_SOME_TYPE
> "SOME_TYPE" refers to a *host* type, whereas the gcc_jit_type parameter
> expresses the *target* type.
>
> So arguably we don't need lots of support for different host types: if
> you need to populate an int32_t constant, you can do:
>
> gcc_jit_type *t_int_32_t =
> gcc_jit_context_get_int_type (ctxt,
> sizeof (int32_t), /* int num_bytes */
> 1); /* int is_signed*/
>
>
> to get the target type (assuming sizeof (int32_t) on the host is the
> same as that on the target, which for these types is of course the
> case).
I am not sure to follow you... What make you believe that sizeof(int32_t)
is always the same for host and target? IIRC an hypothetical implementation
with char of 32 bits and sizeof(char) = sizeof(short) = sizeof(int) =
sizeof(long) = 1
and with sizeof(long long) = 2 would be conforming to the C99 standard.
BTW, I don't care about such pathological cases. My usage of GCCJIT is a
purely JIT thing on x86-64
with both target and host being the same architecture & ABI (x86-64 ...)....
>
> To build an rvalue for a specific int constant we can then simply use a
> big enough host type, say your new long long entrypoint:
>
> gcc_jit_rvalue *rval_const =
> gcc_jit_context_new_rvalue_from_long_long (ctxt,
> t_int32_t,
> INT32_MAX);
>
> given that INT32_MAX will fit in a host "long long".
>
> and indeed if you're using the C++ bindings you can get at integer types
> via some template "magic" like this:
>
> gccjit::type t_int_32_t = ctxt.get_int_type <int32_t> ();
>
> and then the rvalue like this:
>
> gccjit::rvalue rval_const = ctxt.new_rvalue (t_int_32_t,
> INT32_MAX);
>
>
> We have e.g.:
>
> extern gcc_jit_rvalue *
> gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
> gcc_jit_type *numeric_type,
> long long value);
>
> and these might be better described as:
>
>
> extern gcc_jit_rvalue *
> gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
> gcc_jit_type *target_numeric_type,
> long long host_value);
>
> I mention it now in the hope that it clarifies things. You don't need
> to actually make that change - that will be for when we add support for
> cross-compilation.
>
>> The patch is passing the test that I have added, on Debian/x86-64.
> Please run the full "make check-jit", not just the new test.
Will do.
>> Comments are welcome, including perhaps an Ok for trunk...
> Thanks for working on this.
>
> The patch isn't ready for trunk yet: I'd prefer you added "long long"
> and "unsigned long long", and dropped the other new types as noted
> above. Also the patch is at least missing documentation and some test
> coverage, plus some additional points noted below.
>
> FWIW I've fleshed out the notes on submitting jit patches somewhat, and
> they're in the docs now, see:
> https://gcc.gnu.org/onlinedocs/jit/internals/index.html#submitting-patches
>
>
>> Index: gcc/jit/jit-playback.c
>> ===================================================================
>> --- gcc/jit/jit-playback.c (revision 225860)
>> +++ gcc/jit/jit-playback.c (working copy)
>> @@ -1,4 +1,5 @@
>> -/* Internals of libgccjit: classes for playing back recorded API
>> calls.
>> +/* This jit-playback.c file is in -*- C++ -*- ...
>> + Internals of libgccjit: classes for playing back recorded API
>> calls.
> Is the format of these headers designed to be picked up by editors, e.g.
> emacs?
Yes, at least for me. But we really need a comment telling that it is a
C++ file.
(My opinion is that *.c extension for C++ code is insane, but I am in
the minority)
I might happen to pretend understanding that .c extension is a must-have
in GCC
(actually I don't believe that), but in new files which are C++ we
should mention that there are C++ code...., please! (at least for those
newbies who might look into that file)
Do you prefer a comment like
/* This jit-playback.c is in C++99. ...
> Please can you use C syntax for long long literals in the debug string
> (see
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66811 ).
Will try my best to do that....
>> /* The get_wide_int specialization for <long>. */
>>
>> template <>
>> @@ -4171,6 +4189,16 @@ memento_of_new_rvalue_from_const
>> <long>::get_wide_
>> return true;
>> }
>>
>> +/* The get_wide_int specialization for <long long>. */
>> +
>> +template <>
>> +bool
>> +memento_of_new_rvalue_from_const <long long>::get_wide_int (wide_int
>> *out) const
>> +{
>> + *out = wi::shwi (m_value, sizeof (m_value) * 8);
The <long> case that was already there contains the 8 as a magical
constant. I have no idea why. You probably wrote that for the existing
memento_of_new_rvalue_from_const <long>::get_wide_int
> Where does this "8" come from? (it looks like a "magic constant").
I have no idea. But the author of the <long> case should know. Ask him :-)
I guess it might be the number of bits in a char....
Cheers
--
Basile STARYNKEVITCH http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mine, sont seulement les miennes} ***
next prev parent reply other threads:[~2015-07-17 17:30 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <55A6A421.8060707@starynkevitch.net>
2015-01-01 0:00 ` PATCH " David Malcolm
2015-01-01 0:00 ` [PATCH, committed] jit: Add guide for submitting patches to jit docs David Malcolm
2015-01-01 0:00 ` PATCH trunk GCCJIT: adding gcc_jit_context_new_rvalue_from_long_long, etc Basile Starynkevitch
2015-01-01 0:00 ` David Malcolm
2015-01-01 0:00 ` Basile Starynkevitch
2015-01-01 0:00 ` Basile Starynkevitch
2015-01-01 0:00 ` David Malcolm
2015-01-01 0:00 ` PATCH (v2) " Basile Starynkevitch
2015-01-01 0:00 ` David Malcolm
2015-01-01 0:00 ` Basile Starynkevitch [this message]
2015-01-01 0:00 ` David Malcolm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55A93BAE.7060403@starynkevitch.net \
--to=basile@starynkevitch.net \
--cc=dmalcolm@redhat.com \
--cc=jit@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).