RE: Machine Mode ICE in RISC-V when LTO

["Li, Pan2" <pan2.li@intel.com>]

Thanks Thomas for the information, great to learn you have a fix WIP.

> ... is not sufficient: that runs into GTY issues, as the current
> 'unsigned char *lto_mode_identity_table' is (mis-)classified by
> 'gengtype' as a C string.  This happens to work for this case, but still
> isn't right, and only works for 'char *' but not 'short *' etc

Does it reports something like " gcc/lto-streamer.h:599: field `(*x).mode_table' is pointer to unimplemented type" when changing to short *?

>    -return (machine_mode) ib->file_data->mode_table[ix];
>    +return ib->file_data->mode_table ? ib->file_data->mode_table[ix] : ix;

Got the point and the mode_table is constant up to a point.

Pan

-----Original Message-----
From: Thomas Schwinge <thomas@codesourcery.com> 
Sent: Thursday, August 10, 2023 9:24 PM
To: Li, Pan2 <pan2.li@intel.com>; Richard Biener <richard.guenther@gmail.com>; Jakub Jelinek <jakub@redhat.com>
Cc: gcc-patches@gcc.gnu.org; richard.sandiford@arm.com; kito.cheng@gmail.com; Jeff Law <jeffreyalaw@gmail.com>; juzhe.zhong@rivai.ai; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: RE: Machine Mode ICE in RISC-V when LTO

Hi!

On 2023-08-10T12:25:36+0000, "Li, Pan2" <pan2.li@intel.com> wrote:
> Thanks Richard for comment, let me try to promote the table to unsigned short.

I have WIP work for this issue -- which I'd already raised a month ago:
<https://inbox.sourceware.org/87o7kxuq9s.fsf@euler.schwinge.homeip.net>:

On 2023-06-30T13:46:07+0200, Thomas Schwinge <thomas@codesourcery.com> wrote:
> In particular, the 'lto_mode_identity_table' changes would seem necessary
> to keep standard LTO ('-flto') functional for large 'machine_mode' size?

... which is exactly the problem you've now run into?

However, a simple:

    -GTY(()) const unsigned char *lto_mode_identity_table;
    +GTY(()) const unsigned short *lto_mode_identity_table;

..., or:

    -GTY(()) const unsigned char *lto_mode_identity_table;
    +GTY(()) const machine_mode *lto_mode_identity_table;

... is not sufficient: that runs into GTY issues, as the current
'unsigned char *lto_mode_identity_table' is (mis-)classified by
'gengtype' as a C string.  This happens to work for this case, but still
isn't right, and only works for 'char *' but not 'short *' etc.  I have
WIP work to tighten that.  ..., which got me into other GTY issues, and
so on...  ;-) (Richard already ACKed and I pushed some of the
prerequisite changes, but there's more to come.)  I'm still planning on
resolving all that mess, but I'm tight on time right now.

However, I have a different proposal, which should address your current
issue: simply, get rid of the 'lto_mode_identity_table', which is just
that: a 1-to-1 mapping of array index to value.  Instead, in
'gcc/lto/lto-common.cc:lto_file_finalize', for '!ACCEL_COMPILER', set
'file_data->mode_table = NULL', and in the users (only
'gcc/tree-streamer.h:bp_unpack_machine_mode'?), replace (untested):

    -return (machine_mode) ib->file_data->mode_table[ix];
    +return ib->file_data->mode_table ? ib->file_data->mode_table[ix] : ix;

Jakub, as the original author of 'lto_mode_identity_table' (see
commit db847fa8f2cca6139188b8dfa0a7064319b19193 (Subversion r221005)), is
there any reason not to do it this way?


Grüße
 Thomas


> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Thursday, August 10, 2023 7:08 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: richard.sandiford@arm.com; Thomas Schwinge <thomas@codesourcery.com>; jakub@redhat.com; kito.cheng@gmail.com; Jeff Law <jeffreyalaw@gmail.com>; juzhe.zhong@rivai.ai; Wang, Yanzhang <yanzhang.wang@intel.com>
> Subject: Re: Machine Mode ICE in RISC-V when LTO
>
> On Thu, Aug 10, 2023 at 10:19 AM Li, Pan2 <pan2.li@intel.com> wrote:
>>
>> Hi all,
>>
>>
>>
>> Recently I found there is still some issues for the machine mode with LTO part by fixing one
>>
>> ICE (only when compile with LTO) in RISC-V backend in , aka below case.
>>
>>
>>
>> >> ../__RISC-V_INSTALL___/bin/riscv64-unknown-elf-g++ -O2 -flto gcc/testsuite/g++.dg/torture/vshuf-v4df.C -o test.elf
>>
>> during RTL pass: expand
>>
>> gcc/testsuite/g++.dg/torture/vshuf-main.inc: In function 'main':
>>
>> gcc/testsuite/g++.dg/torture/vshuf-main.inc:15:9: internal compiler error: in as_a, at machmode.h:381
>>
>>    15 |       V r = __builtin_shuffle(in1[i], mask1[i]);
>>
>>       |         ^
>>
>> 0x7e5b8e scalar_int_mode as_a<scalar_int_mode>(machine_mode)
>>
>>         ../.././gcc/gcc/machmode.h:381
>>
>> 0x7eabdb scalar_mode as_a<scalar_mode>(machine_mode)
>>
>>         ../.././gcc/gcc/expr.cc:332
>>
>> 0x7eabdb convert_mode_scalar
>>
>>         ../.././gcc/gcc/expr.cc:325
>>
>> 0xb8485b store_expr(tree_node*, rtx_def*, int, bool, bool)
>>
>>         ../.././gcc/gcc/expr.cc:6413
>>
>> 0xb8a556 store_field
>>
>>         ../.././gcc/gcc/expr.cc:7648
>>
>> 0xb88f27 store_constructor(tree_node*, rtx_def*, int, poly_int<2u, long>, bool)
>>
>>         ../.././gcc/gcc/expr.cc:7588
>>
>> 0xb8b8b8 expand_constructor
>>
>>         ../.././gcc/gcc/expr.cc:8931
>>
>> 0xb76bc7 expand_expr_real_1(tree_node*, rtx_def*, machine_mode, expand_modifier, rtx_def**, bool)
>>
>>         ../.././gcc/gcc/expr.cc:11170
>>
>> 0xb77ef7 expand_expr_real_1(tree_node*, rtx_def*, machine_mode, expand_modifier, rtx_def**, bool)
>>
>>         ../.././gcc/gcc/expr.cc:10809
>>
>> 0xb83a80 store_expr(tree_node*, rtx_def*, int, bool, bool)
>>
>>         ../.././gcc/gcc/expr.cc:6325
>>
>> 0xb851d9 expand_assignment(tree_node*, tree_node*, bool)
>>
>>         ../.././gcc/gcc/expr.cc:6043
>>
>> 0xa48717 expand_gimple_stmt_1
>>
>>         ../.././gcc/gcc/cfgexpand.cc:3946
>>
>> 0xa48717 expand_gimple_stmt
>>
>>         ../.././gcc/gcc/cfgexpand.cc:4044
>>
>> 0xa4d030 expand_gimple_basic_block
>>
>>         ../.././gcc/gcc/cfgexpand.cc:6096
>>
>> 0xa4efd6 execute
>>
>>         ../.././gcc/gcc/cfgexpand.cc:6831
>>
>>
>>
>> I double checked the reason that comes from we add even more machine modes in the RISC-V backend,
>>
>> and then did some investigation for the root cause. It should be related to the mode_table, as well as the
>>
>> bp_unpack_machine_mode.
>>
>>
>>
>> In lto_fe_init:
>>
>>    unsigned char *table
>>
>>     = ggc_vec_alloc<unsigned char> (MAX_MACHINE_MODE);
>>
>>
>>
>>    for (int m = 0; m < MAX_MACHINE_MODE; m++)
>>
>> table[m] = m;                                                              <== May overflow here given MAX_MACHINE_MODE > 256 and table[m] is unsigned char.
>>
>>
>>
>> in bp_unpack_machine_mode:
>>
>>    unsigned ix = bp_unpack_enum (bp, machine_mode, last);
>>
>>   return (machine_mode) ib->file_data->mode_table[ix];  <== May return truncated mode here.
>>
>>
>>
>> To validate this idea, I tried below hack code for double checking and then there is no ICE anymore, which indicates
>>
>> the problem here as I bet. However, the lto is quite complicated and I am not sure how to fix it in the right way.
>>
>>
>>
>> +    = ggc_vec_alloc<unsigned char> (MAX_MACHINE_MODE * 2);
>>
>> …
>>
>> +    ((unsigned short *)table)[m] = m;
>>
>> …
>>
>> +   return (machine_mode) ((unsigned short *)ib->file_data->mode_table)[ix];
>>
>>
>>
>> Besides, I also tried to change the mode_table from char * to short * but got one weird error when building as below.
>>
>>
>>
>> gcc/lto-streamer.h:599: field `(*x).mode_table' is pointer to unimplemented type
>
> We still have some places using an array of char for the mode table.
> The above is assigned
> to lto_mode_identity_table which ends up in
> lto_file_decl_data::mode_table.  I think those
> need to be all promoted to unsigned short.
>
> Richard.
>
>>
>>
>> Pan
>>
>>
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955