Hi! On 2023-08-10T12:25:36+0000, "Li, Pan2" <pan2...@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....@euler.schwinge.homeip.net>: On 2023-06-30T13:46:07+0200, Thomas Schwinge <tho...@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.guent...@gmail.com> > Sent: Thursday, August 10, 2023 7:08 PM > To: Li, Pan2 <pan2...@intel.com> > Cc: richard.sandif...@arm.com; Thomas Schwinge <tho...@codesourcery.com>; > ja...@redhat.com; kito.ch...@gmail.com; Jeff Law <jeffreya...@gmail.com>; > juzhe.zh...@rivai.ai; Wang, Yanzhang <yanzhang.w...@intel.com> > Subject: Re: Machine Mode ICE in RISC-V when LTO > > On Thu, Aug 10, 2023 at 10:19 AM Li, Pan2 <pan2...@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
