Re: [PATCH 10/52] jit: Replace uses of {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE

Re: [PATCH 10/52] jit: Replace uses of {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE

[David Malcolm]

On Sun, 2024-06-02 at 22:01 -0500, Kewen Lin wrote:
> Joseph pointed out "floating types should have their mode,
> not a poorly defined precision value" in the discussion[1],
> as he and Richi suggested, the existing macros
> {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE will be replaced with a
> hook mode_for_floating_type.  Unlike the other FEs, for the
> uses in recording::memento_of_get_type::get_size, since
> {float,{,long_}double}_type_node haven't been initialized
> yet, this is to replace {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE
> with calling hook targetm.c.mode_for_floating_type.
> 
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651209.html
> 
> gcc/jit/ChangeLog:
> 
>         * jit-recording.cc
> (recording::memento_of_get_type::get_size): Update
>         macros {FLOAT,DOUBLE,LONG_DOUBLE}_TYPE_SIZE by calling
>         targetm.c.mode_for_floating_type with
>         TI_{FLOAT,DOUBLE,LONG_DOUBLE}_TYPE.
> ---
>  gcc/jit/jit-recording.cc | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
> index 68a2e860c1f..7719b898e57 100644
> --- a/gcc/jit/jit-recording.cc
> +++ b/gcc/jit/jit-recording.cc
> @@ -21,7 +21,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "config.h"
>  #include "system.h"
>  #include "coretypes.h"
> -#include "tm.h"
> +#include "target.h"
>  #include "pretty-print.h"
>  #include "toplev.h"
>  
> @@ -2353,6 +2353,7 @@ size_t
>  recording::memento_of_get_type::get_size ()
>  {
>    int size;
> +  machine_mode m;
>    switch (m_kind)
>      {
>      case GCC_JIT_TYPE_VOID:
> @@ -2399,13 +2400,16 @@ recording::memento_of_get_type::get_size ()
>        size = 128;
>        break;
>      case GCC_JIT_TYPE_FLOAT:
> -      size = FLOAT_TYPE_SIZE;
> +      m = targetm.c.mode_for_floating_type (TI_FLOAT_TYPE);
> +      size = GET_MODE_PRECISION (m).to_constant ();
>        break;
>      case GCC_JIT_TYPE_DOUBLE:
> -      size = DOUBLE_TYPE_SIZE;
> +      m = targetm.c.mode_for_floating_type (TI_DOUBLE_TYPE);
> +      size = GET_MODE_PRECISION (m).to_constant ();
>        break;
>      case GCC_JIT_TYPE_LONG_DOUBLE:
> -      size = LONG_DOUBLE_TYPE_SIZE;
> +      m = targetm.c.mode_for_floating_type (TI_LONG_DOUBLE_TYPE);
> +      size = GET_MODE_PRECISION (m).to_constant ();
>        break;
>      case GCC_JIT_TYPE_SIZE_T:
>        size = MAX_BITS_PER_WORD;

[CCing jit mailing list]

Thanks for the patch; sorry for the delay in responding.

Did your testing include jit?  Note that --enable-languages=all does
*not* include it (due to it needing --enable-host-shared).

The jit::recording code runs *very* early - before toplev::main.  For
example, a call to gcc_jit_type_get_size can trigger the above code
path before toplev::main has run.

target.h says each target should have a:

  struct gcc_target targetm = TARGET_INITIALIZER;

Has targetm.c.mode_for_floating_type been initialized enough by that
static initialization?  Could the mode_for_floating_type hook be
relying on some target-specific dynamic initialization that hasn't run
yet?  (e.g. taking account of command-line options?)

Dave

Re: [PATCH 10/52] jit: Replace uses of {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE

[Kewen.Lin]

Hi David,

on 2024/6/13 21:44, David Malcolm wrote:
> On Sun, 2024-06-02 at 22:01 -0500, Kewen Lin wrote:
>> Joseph pointed out "floating types should have their mode,
>> not a poorly defined precision value" in the discussion[1],
>> as he and Richi suggested, the existing macros
>> {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE will be replaced with a
>> hook mode_for_floating_type.  Unlike the other FEs, for the
>> uses in recording::memento_of_get_type::get_size, since
>> {float,{,long_}double}_type_node haven't been initialized
>> yet, this is to replace {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE
>> with calling hook targetm.c.mode_for_floating_type.
>>
>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651209.html
>>
>> gcc/jit/ChangeLog:
>>
>>         * jit-recording.cc
>> (recording::memento_of_get_type::get_size): Update
>>         macros {FLOAT,DOUBLE,LONG_DOUBLE}_TYPE_SIZE by calling
>>         targetm.c.mode_for_floating_type with
>>         TI_{FLOAT,DOUBLE,LONG_DOUBLE}_TYPE.
>> ---
>>  gcc/jit/jit-recording.cc | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
>> index 68a2e860c1f..7719b898e57 100644
>> --- a/gcc/jit/jit-recording.cc
>> +++ b/gcc/jit/jit-recording.cc
>> @@ -21,7 +21,7 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "config.h"
>>  #include "system.h"
>>  #include "coretypes.h"
>> -#include "tm.h"
>> +#include "target.h"
>>  #include "pretty-print.h"
>>  #include "toplev.h"
>>  
>> @@ -2353,6 +2353,7 @@ size_t
>>  recording::memento_of_get_type::get_size ()
>>  {
>>    int size;
>> +  machine_mode m;
>>    switch (m_kind)
>>      {
>>      case GCC_JIT_TYPE_VOID:
>> @@ -2399,13 +2400,16 @@ recording::memento_of_get_type::get_size ()
>>        size = 128;
>>        break;
>>      case GCC_JIT_TYPE_FLOAT:
>> -      size = FLOAT_TYPE_SIZE;
>> +      m = targetm.c.mode_for_floating_type (TI_FLOAT_TYPE);
>> +      size = GET_MODE_PRECISION (m).to_constant ();
>>        break;
>>      case GCC_JIT_TYPE_DOUBLE:
>> -      size = DOUBLE_TYPE_SIZE;
>> +      m = targetm.c.mode_for_floating_type (TI_DOUBLE_TYPE);
>> +      size = GET_MODE_PRECISION (m).to_constant ();
>>        break;
>>      case GCC_JIT_TYPE_LONG_DOUBLE:
>> -      size = LONG_DOUBLE_TYPE_SIZE;
>> +      m = targetm.c.mode_for_floating_type (TI_LONG_DOUBLE_TYPE);
>> +      size = GET_MODE_PRECISION (m).to_constant ();
>>        break;
>>      case GCC_JIT_TYPE_SIZE_T:
>>        size = MAX_BITS_PER_WORD;
> 
> [CCing jit mailing list]
> 
> Thanks for the patch; sorry for the delay in responding.
> 
> Did your testing include jit?  Note that --enable-languages=all does
> *not* include it (due to it needing --enable-host-shared).

Thanks for the hints!  Yes, as noted in the cover letter, I did test jit.
Initially I used TYPE_PRECISION ({float,{long_,}double_type_node) to
replace these just like what I proposed for the other FE changes, but the
testing showed some failures on test-combination.c etc., by looking into
them, I realized that this call recording::memento_of_get_type::get_size
can happen before when we set up those type nodes.  Then I had to use the
current approach with the new hook, it made all failures gone (no
regressions).  btw, test result comparison showed some more lines with
"NA->PASS: test-threads.c.exe", since it's positive, I didn't look into
it.

> 
> The jit::recording code runs *very* early - before toplev::main.  For
> example, a call to gcc_jit_type_get_size can trigger the above code
> path before toplev::main has run.
> 
> target.h says each target should have a:
> 
>   struct gcc_target targetm = TARGET_INITIALIZER;
> 
> Has targetm.c.mode_for_floating_type been initialized enough by that
> static initialization?  

It depends on how to define "enough".  The hook has been initialized
as you pointed out, I just debugged it and confirmed target specific
hook was called as expected (rs6000_c_mode_for_floating_type on Power)
when this jit::recording function gets called.  If "enough" refers to
something like command line options, it's not ready.

> Could the mode_for_floating_type hook be
> relying on some target-specific dynamic initialization that hasn't run
> yet?  (e.g. taking account of command-line options?)
> 

Yes, it could.  Like rs6000 port, the hook checks rs6000_long_double_type_size
for long double (it's related to command line option -mlong-double-x) and
some other targets like i386, also would like to check TARGET_LONG_DOUBLE_64
and TARGET_LONG_DOUBLE_128.  But I think it isn't worse than before, without
this change (with the previous macro), we used to define the macro with
the things related to this command line options, which are still not ready.

#define LONG_DOUBLE_TYPE_SIZE rs6000_long_double_type_size

I debugged the code, jit::recording will see rs6000_long_double_type_size
with the static initialized value zero, it means that the function 
recording::memento_of_get_type::get_size would get zero byte size for the
type (I assume that it's unexpected for the code?).  With this new hook,
although it can provide not exact type size (can be off from the one
specified by command line), it returns a reasonable size (comparing with
the zero size).  From this perspective, it's slightly better?

+  if (ti == TI_LONG_DOUBLE_TYPE)
+    return rs6000_long_double_type_size == FLOAT_PRECISION_TFmode ? TFmode
+								  : DFmode;

BR,
Kewen