PATCH (v2) trunk GCCJIT: adding gcc_jit_context_new_rvalue_from_long_long, etc...

[Basile Starynkevitch]

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} ***