On Mon, Nov 4, 2024 at 11:19 PM Jakub Jelinek <ja...@redhat.com> wrote: > > On Mon, Nov 04, 2024 at 10:58:14AM +0100, Richard Biener wrote: > > > Regarding memory usage implications of this change. It does make a lot of > > > data structures larger. Here is what it does on x86-64. The base sizes of > > > some > > > types change as follows: > > > > > > location_t: From 4 to 8 (+100%) > > > line_map_ordinary: From 24 to 32 (+33%) > > > line_map_macro: From 32 to 40 (+25%) > > > cpp_token: From 24 to 32 (+33%) > > > cpp_macro: From 48 to 56 (+17%) > > > tree_exp: From 32 to 40 (+25%) > > > gimple: From 40 to 48 (+20%) > > > > All GENERIC decls (tree_decl_minimal) will grow by 8 bytes (and we have 4 > > bytes > > unused padding). On the GIMPLE side it's probably a "hooray - more space > > for > > flags!". It does feel a bit backwards because the savings we got by making > > the TREE_BLOCK part of the location. > > I think we need to carefully look at each of the structs with location_t > that is used widely. > E.g. for tree_exp, I think we just had a padding there up to and including > GCC 13, and in GCC 14+, while EXPR_COND_UID is computed unconditionally, it > is ignored unless -fcondition-coverage which is not enabled by default and > I think it isn't commonly used and is used solely on COND_EXPRs. > Furthermore, for gimple we don't waste precious space for it and use on the > side hash map, so why not use similar hash-map for COND_EXPRs during > gimplification too (and only for -fcondition-coverage)? > That would keep tree_exp at the same size as before.
Good idea. > Other than that, guess gimple is the most important, especially for LTO. > Except for GIMPLE_CALL/ASM/SWITCH, most GIMPLE_* codes have very low num_ops, > so perhaps having 32-bit num_ops is a waste. All non-GIMPLE_ASSIGN_SINGLE have the possibility of much saving: - as you say num_ops is either 2, 3 or 4 - they are unnecessarily using gimple_statement_with_memory_ops_base, no vuse/def on non-SINGLE For GIMPLE_ASSIGN_SINGLE num_ops is 2. So - ICK - the "best" way would be to split gassign into gsingle_assign (gload, gstore, gcopy, glegacy?) and goperation. Currently gassign has gimple_statement_with_memory_ops as base, I'm not sure we can keep gassign as a base of the two new classes and thus make it have gimple_statement_with_ops_base as base. A "cheap" way would be to scrap _with_memory_ops and put vuse/vdef in two trailing ops[] positions at ops[num_ops] and ops[num_ops+1], making gimple_vuse and friends a bit more expensive and complicated to verify (we have to check for GIMPLE_ASSIGN_SINGLE and always allocate for those). Generally num_ops has to be able to be large (for calls and switch), and given num_ops is currently in the base and also accessed with just gimple * it's difficult to move it down the class hierarchy without using virtual functions to access it or make the access expensive (switch on gimple_code and up-cast). That said, for gassign num_ops is redundant information, and in general 32bits is overkill, 16bits should be OK - salvaging 'subcode' might be possible as well by moving it where needed. > Though we'd need 3 bits to represent those, or at least 2 if GIMPLE_COND > has it somewhere else. GIMPLE_COND always has two ops, num_ops is redundant. I think there's the possibility to get back the memory on the GIMPLE side but I wouldn't make this a requirement for this patch. > cpp_token hurts too, at least for C++, though I'm not sure what to do that. > While most of the union members are just 32 or 64-bit, there are two with > 32-bit and 64-bit data (that could be in theory handled by having the 32-bit > stuff in some flags field and union just the 64-bit) but then for identifiers > we want 2 pointers. We could dynamically allocate cpp_token and run-length encode location_t, making it effectively less than 64bit. For cpp_token we'd have to split it into a low 32bit at the existing location and optionally allocated upper 32bits. But I'm honestly not sure this is worth the trouble - it would also restrict how we allocate the bits of location_t. We might want to dynamically allocate cpp_token in general given the current 24 bytes are only for the largest union members - but I have no statistics on the token distribution and no idea if we use an array of cpp_token somewhere which would rule this out. Richard. > Jakub >
