Hi Richard,
Thanks so much for your great review! Richard Biener <rguent...@suse.de> writes: > On Wed, 23 Aug 2023, Jiufu Guo wrote: > >> >> Hi, >> >> I just updated the patch. We could review this one. >> >> Compare with previous patch: >> https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627287.html >> This version: >> * Supports bitfield access from one register. >> * Allow return scalar registers cleaned via contructor. >> >> Bootstrapped and regtested on x86_64-redhat-linux, and >> powerpc64{,le}-linux-gnu. >> >> Is it ok for trunk? > > Some comments inline - not a full review (and sorry for the delay). > >> >> PR target/65421 >> PR target/69143 >> >> gcc/ChangeLog: >> >> * cfgexpand.cc (extract_bit_field): Extern declare. >> (struct access): New class. >> (struct expand_sra): New class. >> (expand_sra::build_access): New member function. >> (expand_sra::visit_base): Likewise. >> (expand_sra::analyze_default_stmt): Likewise. >> (expand_sra::analyze_assign): Likewise. >> (expand_sra::add_sra_candidate): Likewise. >> (expand_sra::collect_sra_candidates): Likewise. >> (expand_sra::valid_scalariable_accesses): Likewise. >> (expand_sra::prepare_expander_sra): Likewise. >> (expand_sra::expand_sra): Class constructor. >> (expand_sra::~expand_sra): Class destructor. >> (expand_sra::get_scalarized_rtx): New member function. >> (extract_one_reg): New function. >> (extract_bitfield): New function. >> (expand_sra::scalarize_access): New member function. >> (expand_sra::scalarize_accesses): New member function. >> (get_scalar_rtx_for_aggregate_expr): New function. >> (set_scalar_rtx_for_aggregate_access): New function. >> (set_scalar_rtx_for_returns): New function. >> (expand_return): Call get_scalar_rtx_for_aggregate_expr. >> (expand_debug_expr): Call get_scalar_rtx_for_aggregate_expr. >> (pass_expand::execute): Update to use the expand_sra. >> * expr.cc (get_scalar_rtx_for_aggregate_expr): Extern declare. >> (expand_assignment): Call get_scalar_rtx_for_aggregate_expr. >> (expand_expr_real): Call get_scalar_rtx_for_aggregate_expr. >> * function.cc (set_scalar_rtx_for_aggregate_access): Extern declare. >> (set_scalar_rtx_for_returns): Extern declare. >> (assign_parm_setup_block): Call set_scalar_rtx_for_aggregate_access. >> (assign_parms): Call set_scalar_rtx_for_aggregate_access. >> (expand_function_start): Call set_scalar_rtx_for_returns. >> * tree-sra.h (struct base_access): New class. >> (struct default_analyzer): New class. >> (scan_function): New function template. >> >> gcc/testsuite/ChangeLog: >> >> * g++.target/powerpc/pr102024.C: Updated. >> * gcc.target/powerpc/pr108073.c: New test. >> * gcc.target/powerpc/pr65421-1.c: New test. >> * gcc.target/powerpc/pr65421-2.c: New test. >> >> --- >> gcc/cfgexpand.cc | 474 ++++++++++++++++++- >> gcc/expr.cc | 29 +- >> gcc/function.cc | 28 +- >> gcc/tree-sra.h | 77 +++ >> gcc/testsuite/g++.target/powerpc/pr102024.C | 2 +- >> gcc/testsuite/gcc.target/powerpc/pr108073.c | 29 ++ >> gcc/testsuite/gcc.target/powerpc/pr65421-1.c | 6 + >> gcc/testsuite/gcc.target/powerpc/pr65421-2.c | 32 ++ >> 8 files changed, 668 insertions(+), 9 deletions(-) >> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr108073.c >> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr65421-1.c >> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr65421-2.c >> >> diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc >> index >> edf292cfbe95ac2711faee7769e839cb4edb0dd3..385b6c781aa2805e7ca40293a0ae84f87e23e0b6 >> 100644 >> --- a/gcc/cfgexpand.cc >> +++ b/gcc/cfgexpand.cc >> @@ -74,6 +74,7 @@ along with GCC; see the file COPYING3. If not see >> #include "output.h" >> #include "builtins.h" >> #include "opts.h" >> +#include "tree-sra.h" >> >> /* Some systems use __main in a way incompatible with its use in gcc, in >> these >> cases use the macros NAME__MAIN to give a quoted symbol and SYMBOL__MAIN >> to >> @@ -97,6 +98,468 @@ static bool defer_stack_allocation (tree, bool); >> >> static void record_alignment_for_reg_var (unsigned int); >> >> +extern rtx extract_bit_field (rtx, poly_uint64, poly_uint64, int, rtx, >> + machine_mode, machine_mode, bool, rtx *); > > belongs in some header Thanks. Will update. > >> + >> +/* For light SRA in expander about paramaters and returns. */ >> +struct access : public base_access >> +{ >> + /* The rtx for the access: link to incoming/returning register(s). */ >> + rtx rtx_val; >> +}; >> + >> +typedef struct access *access_p; >> + >> +struct expand_sra : public default_analyzer >> +{ > > Both 'base_access' and 'default_analyzer' need a more specific > name I think. Just throwing in two names here, > 'sra_access_base' and 'sra_default_analyzer' Thanks! > >> + expand_sra (); >> + ~expand_sra (); >> + >> + /* Now use default APIs, no actions for >> + pre_analyze_stmt, analyze_return. */ >> + >> + /* overwrite analyze_default_stmt. */ >> + void analyze_default_stmt (gimple *); >> + >> + /* overwrite analyze phi,call,asm . */ >> + void analyze_phi (gphi *stmt) { analyze_default_stmt (stmt); }; >> + void analyze_call (gcall *stmt) { analyze_default_stmt (stmt); }; >> + void analyze_asm (gasm *stmt) { analyze_default_stmt (stmt); }; > > that looks odd I would define another function with a name like: protect_mem_access_in_stmt (gimple*) "analyze_default_stmt and analyze_phi" call this function. analyze_call and analyze_asm would be updated to analyze sra accesses on call/asm stmt later. > >> + /* overwrite analyze_assign. */ >> + void analyze_assign (gassign *); >> + >> + /* Compute the scalar rtx(s) for all access of BASE from a parrallel >> REGS. */ >> + bool scalarize_accesses (tree base, rtx regs); >> + /* Return the scalarized rtx for EXPR. */ >> + rtx get_scalarized_rtx (tree expr); >> + >> +private: >> + void prepare_expander_sra (void); >> + >> + /* Return true if VAR is a candidate for SRA. */ >> + bool add_sra_candidate (tree var); >> + >> + /* Collect the parameter and returns with type which is suitable for >> + scalarization. */ >> + bool collect_sra_candidates (void); >> + >> + /* Return true if EXPR has interesting access to the sra candidates, >> + and created access, return false otherwise. */ >> + access_p build_access (tree expr, bool write); >> + >> + /* Check if the accesses of BASE are scalarizbale. >> + Now support the parms only with reading or returns only with writing. >> */ >> + bool valid_scalariable_accesses (vec<access_p> *access_vec, bool is_parm); >> + >> + /* Compute the scalar rtx for one access ACC from a parrallel REGS. */ >> + bool scalarize_access (access_p acc, rtx regs); >> + >> + /* Callback of walk_stmt_load_store_addr_ops, used to remove >> + unscalarizable accesses. */ >> + static bool visit_base (gimple *, tree op, tree, void *data); >> + >> + /* Expr (tree) -> Scalarized value (rtx) map. */ >> + hash_map<tree, rtx> *expr_rtx_vec; >> + >> + /* Base (tree) -> Vector (vec<access_p> *) map. */ >> + hash_map<tree, auto_vec<access_p> > *base_access_vec; >> +}; >> + >> +access_p >> +expand_sra::build_access (tree expr, bool write) >> +{ >> + enum tree_code code = TREE_CODE (expr); >> + if (code != VAR_DECL && code != PARM_DECL && code != COMPONENT_REF >> + && code != ARRAY_REF && code != ARRAY_RANGE_REF) >> + return NULL; >> + >> + HOST_WIDE_INT offset, size; >> + bool reverse; >> + tree base = get_ref_base_and_extent_hwi (expr, &offset, &size, &reverse); >> + if (!base || !DECL_P (base)) >> + return NULL; >> + if (storage_order_barrier_p (expr) || TREE_THIS_VOLATILE (expr)) >> + { >> + base_access_vec->remove (base); >> + return NULL; >> + } >> + >> + vec<access_p> *access_vec = base_access_vec->get (base); >> + if (!access_vec) >> + return NULL; >> + >> + /* TODO: support reverse. */ >> + if (reverse || size <= 0 || offset + size > tree_to_shwi (DECL_SIZE >> (base))) >> + { >> + base_access_vec->remove (base); >> + return NULL; >> + } >> + >> + struct access *access = XNEWVEC (struct access, 1); >> + >> + memset (access, 0, sizeof (struct access)); >> + access->offset = offset; >> + access->size = size; >> + access->expr = expr; >> + access->write = write; >> + access->rtx_val = NULL_RTX; >> + >> + access_vec->safe_push (access); > > doesn't look like this shares anything with SRA? For this code in tree-sra and ipa-sra, they are just "**seems**" similar, but they are different, even extracting the "common" code slightly, it may look crushed. I would still try to common the code. > >> + Return access; >> +} >> + >> +bool >> +expand_sra::visit_base (gimple *, tree op, tree, void *data) >> +{ >> + op = get_base_address (op); >> + if (op && DECL_P (op)) >> + { >> + expand_sra *p = (expand_sra *) data; >> + p->base_access_vec->remove (op); >> + } >> + return false; >> +} >> + >> +void >> +expand_sra::analyze_default_stmt (gimple *stmt) >> +{ >> + if (base_access_vec && !base_access_vec->is_empty ()) >> + walk_stmt_load_store_addr_ops (stmt, this, visit_base, visit_base, >> + visit_base); > > comments would be helpful. This seems to remove all bases for > references based on a decl?! Are you really interested in addrs? Yes. In tree-sra/ipa-sra, stmt (assign/call/asm/return) are checked, and may disqualify(remove) the base if risk. This patch is relatively conservative, it only keeps sra candidates on assignment. For example: "%_7=&arg", and %_7 is used for some purpose. For this case, "arg" is addr taken, and then it needs a mem for it, and then it would be at risk to do sra without deep analyzing. > >> +} >> + >> +void >> +expand_sra::analyze_assign (gassign *stmt) >> +{ >> + if (!base_access_vec || base_access_vec->is_empty ()) >> + return; >> + >> + if (gimple_assign_single_p (stmt) && !gimple_clobber_p (stmt)) >> + { >> + tree rhs = gimple_assign_rhs1 (stmt); >> + tree lhs = gimple_assign_lhs (stmt); >> + bool res_r = build_access (rhs, false); >> + bool res_l = build_access (lhs, true); >> + >> + if (res_l || res_r) > > does this assume we can analyze all accesses that are in the > lhs/rhs? Because if we have an aggregate copy there might be one > in both but one(?) might fail? For the light-expand-sra, "lhs" of an assignment which we care about is only "return", or say, assigning to a return-var. "rhs" of an assignment that we care about is a parameter(or part of parameter). It is also ok for aggregate copy between parameter and return-var. > > Why do we remove bases - since this is all about heuristics it should > be OK to "ignore" accesses we cannot / do not analyze? Removing the whole base is simple. To support partial sra, we need more work. - Need more analysis to make sure the scalarized part is not affected by the access to other parts of the same base. - We need to allow the parameter to be stored to stack for non-sra access, and also allow access to the scalarized at the same time. Need to figure out one way to let DECL_RTL registers and DECL_RTL of stack co-exist. > > Why doesn't this use walk_stmt_load_store_addr_ops as well? Two reasons: 1. The callback of this walk has "base" as a parameter, but we need the 'access expr' which has "base, offset, size" info. 2. The most common case for the parameter sra is reading/ writing field(s) of parmater. Then what we care about is: if "rhs/rhs" is a field/member access of an aggregate parameter/return. Then checking "lhs/rhs" directly would be simpler. > >> + return; >> + } >> + >> + analyze_default_stmt (stmt); >> +} >> + >> +/* Return true if VAR is a candidate for SRA. */ >> + >> +bool >> +expand_sra::add_sra_candidate (tree var) >> +{ >> + tree type = TREE_TYPE (var); >> + >> + if (!AGGREGATE_TYPE_P (type) || !tree_fits_shwi_p (TYPE_SIZE (type)) >> + || tree_to_shwi (TYPE_SIZE (type)) == 0 || TREE_THIS_VOLATILE (var) >> + || is_va_list_type (type)) >> + return false; >> + gcc_assert (COMPLETE_TYPE_P (type)); >> + >> + base_access_vec->get_or_insert (var); >> + >> + return true; >> +} >> + >> +bool >> +expand_sra::collect_sra_candidates (void) >> +{ >> + bool ret = false; >> + >> + /* Collect parameters. */ >> + for (tree parm = DECL_ARGUMENTS (current_function_decl); parm; >> + parm = DECL_CHAIN (parm)) >> + ret |= add_sra_candidate (parm); >> + >> + /* Collect VARs on returns. */ >> + if (DECL_RESULT (current_function_decl)) >> + { >> + edge_iterator ei; >> + edge e; >> + FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds) >> + if (greturn *r = safe_dyn_cast<greturn *> (*gsi_last_bb (e->src))) >> + { >> + tree val = gimple_return_retval (r); >> + /* To sclaraized the return, the return value should be only >> + writen, except this return stmt. >> + Then using 'true(write)' to create the access. */ >> + if (val && VAR_P (val)) > > what about RESULT_DECLs? I think you want to check > needs_to_live_in_memory. If RESULT_DECL is on the return-stmts and return via registers, then the DECL_RTL of RESULT_DECL is already as "PARALLEL" code. We already handled this case fine. In this patch, we only need to handle a special case: e.g. D.2872 = XXX; return D.2872. In this case, a VAR(D.2872) is allocated, and it is not the RESULT_DECL, and return-stmt returns the VAR(D.2872). > >> + ret |= add_sra_candidate (val) && build_access (val, true); >> + } >> + } >> + >> + return ret; >> +} >> + >> +bool >> +expand_sra::valid_scalariable_accesses (vec<access_p> *access_vec, bool >> is_parm) >> +{ >> + if (access_vec->is_empty ()) >> + return false; >> + >> + for (unsigned int j = 0; j < access_vec->length (); j++) >> + { >> + struct access *access = (*access_vec)[j]; >> + if (is_parm && access->write) >> + return false; >> + >> + if (!is_parm && !access->write) >> + return false; > > Can you explain why you are making this restriction? With this restriction, it would be safe/simple, and would be able to cover most cases. tree-sra/ipa-sra handle most of the cases already. For writing to parameter: like "a.d += xx;" The SSA code may like: "a$d_4 = a.d; _2 = _1 + a$d_4;" There is no "writing to parameter" anymore. While there are still cases about "writing" to parameter: e.g. "a$d_11 = a.d; a.d = _2; _7 = bar (a)" To support this, we may need more analysis to do. > >> + } >> + >> + return true; >> +} >> + >> +void >> +expand_sra::prepare_expander_sra () >> +{ >> + if (optimize <= 0) >> + return; >> + >> + base_access_vec = new hash_map<tree, auto_vec<access_p> >; >> + expr_rtx_vec = new hash_map<tree, rtx>; >> + >> + collect_sra_candidates (); >> +} >> + >> +expand_sra::expand_sra () : expr_rtx_vec (NULL), base_access_vec (NULL) >> +{ >> + prepare_expander_sra (); > > inline this function here. Thanks. > >> +} >> + >> +expand_sra::~expand_sra () >> +{ >> + if (optimize <= 0) >> + return; >> + delete expr_rtx_vec; >> + expr_rtx_vec = NULL; >> + delete base_access_vec; >> + base_access_vec = NULL; >> +} >> + >> +rtx >> +expand_sra::get_scalarized_rtx (tree expr) >> +{ >> + if (!expr_rtx_vec) >> + return NULL_RTX; >> + rtx *val = expr_rtx_vec->get (expr); >> + return val ? *val : NULL_RTX; >> +} >> + >> +/* Get the register at INDEX from a parallel REGS. */ >> + >> +static rtx >> +extract_one_reg (rtx regs, int index) >> +{ >> + rtx orig_reg = XEXP (XVECEXP (regs, 0, index), 0); >> + if (!HARD_REGISTER_P (orig_reg)) >> + return orig_reg; >> + >> + /* Reading from param hard reg need to be moved to a temp. */ >> + rtx reg = gen_reg_rtx (GET_MODE (orig_reg)); >> + emit_move_insn (reg, orig_reg); >> + return reg; >> +} >> + >> +/* Extract bitfield from REG with SIZE as MODE, start from POS. */ >> + >> +static rtx >> +extract_bitfield (rtx reg, int size, int pos, machine_mode tmode, bool >> unsignedp) > > quite a bad name for extract_bit_field which we already have ... > I wonder why you need this at all. Thanks! extract_bit_field would be fine. > >> +{ >> + scalar_int_mode imode; >> + if (!int_mode_for_mode (tmode).exists (&imode)) >> + return NULL_RTX; >> + >> + machine_mode mode = GET_MODE (reg); >> + bool reverse = false; >> + rtx bfld = extract_bit_field (reg, size, pos, unsignedp, NULL_RTX, mode, >> + imode, reverse, NULL); >> + mode = GET_MODE (bfld); >> + if (mode != imode) >> + bfld = gen_lowpart (imode, bfld); >> + rtx result = gen_reg_rtx (imode); >> + emit_move_insn (result, bfld); >> + >> + if (tmode != imode) >> + result = gen_lowpart (tmode, result); >> + >> + return result; >> +} >> + >> +bool >> +expand_sra::scalarize_access (access_p acc, rtx regs) >> +{ >> + machine_mode expr_mode = TYPE_MODE (TREE_TYPE (acc->expr)); >> + >> + /* mode of mult registers. */ >> + if (expr_mode != BLKmode >> + && known_gt (acc->size, GET_MODE_BITSIZE (word_mode))) >> + return false; >> + > > I'm confused now - you are rewriting RTL? In this patch, yes, the computed rtx(s) are generated and emitted. e.g. %r123=%r3; %r124=%r4#0 "scalarize_access" is called at the time when parameters are set up, and incoming registers are confirmed. Then, "scalarize_accesses" checks the aggregate parameters, and computes scalarized rtx according to the (base, offset, size) of the expression for each access. As you mentioned below, another idea is: here only confirm if accesses can be scalarized or not and set DECL_RTL for the parameter. > >> + /* Compute the position of the access in the whole parallel rtx. */ >> + int start_index = -1; >> + int end_index = -1; >> + HOST_WIDE_INT left_bits = 0; >> + HOST_WIDE_INT right_bits = 0; >> + int cur_index = XEXP (XVECEXP (regs, 0, 0), 0) ? 0 : 1; >> + for (; cur_index < XVECLEN (regs, 0); cur_index++) >> + { >> + rtx slot = XVECEXP (regs, 0, cur_index); >> + HOST_WIDE_INT off = UINTVAL (XEXP (slot, 1)) * BITS_PER_UNIT; >> + machine_mode mode = GET_MODE (XEXP (slot, 0)); >> + HOST_WIDE_INT size = GET_MODE_BITSIZE (mode).to_constant (); >> + if (off <= acc->offset && off + size > acc->offset) >> + { >> + start_index = cur_index; >> + left_bits = acc->offset - off; >> + } >> + if (off + size >= acc->offset + acc->size) >> + { >> + end_index = cur_index; >> + right_bits = off + size - (acc->offset + acc->size); >> + break; >> + } >> + } >> + /* Invalid access possition: padding or outof bound. */ >> + if (start_index < 0 || end_index < 0) >> + return false; >> + >> + /* Need multi-registers in a parallel for the access. */ >> + if (expr_mode == BLKmode || end_index > start_index) >> + { >> + if (left_bits || right_bits) >> + return false; >> + >> + int num_words = end_index - start_index + 1; >> + rtx *tmps = XALLOCAVEC (rtx, num_words); >> + >> + int pos = 0; >> + HOST_WIDE_INT start; >> + start = UINTVAL (XEXP (XVECEXP (regs, 0, start_index), 1)); >> + /* Extract whole registers. */ >> + for (; pos < num_words; pos++) >> + { >> + int index = start_index + pos; >> + rtx reg = extract_one_reg (regs, index); >> + machine_mode mode = GET_MODE (reg); >> + HOST_WIDE_INT off; >> + off = UINTVAL (XEXP (XVECEXP (regs, 0, index), 1)) - start; >> + tmps[pos] = gen_rtx_EXPR_LIST (mode, reg, GEN_INT (off)); >> + } >> + >> + rtx reg = gen_rtx_PARALLEL (expr_mode, gen_rtvec_v (pos, tmps)); >> + acc->rtx_val = reg; >> + return true; >> + } >> + >> + /* Just need one reg for the access. */ >> + if (end_index == start_index && left_bits == 0 && right_bits == 0) >> + { >> + rtx reg = extract_one_reg (regs, start_index); >> + if (GET_MODE (reg) != expr_mode) >> + reg = gen_lowpart (expr_mode, reg); >> + >> + acc->rtx_val = reg; >> + return true; >> + } >> + >> + /* Need to extract part reg for the access. */ >> + if (!acc->write && end_index == start_index) >> + { >> + rtx reg = XEXP (XVECEXP (regs, 0, start_index), 0); >> + bool sgn = TYPE_UNSIGNED (TREE_TYPE (acc->expr)); >> + acc->rtx_val >> + = extract_bitfield (reg, acc->size, left_bits, expr_mode, sgn); >> + if (acc->rtx_val) >> + return true; >> + } >> + >> + return false; >> +} >> + >> +bool >> +expand_sra::scalarize_accesses (tree base, rtx regs) >> +{ >> + if (!base_access_vec) >> + return false; >> + vec<access_p> *access_vec = base_access_vec->get (base); >> + if (!access_vec) >> + return false; >> + bool is_parm = TREE_CODE (base) == PARM_DECL; >> + if (!valid_scalariable_accesses (access_vec, is_parm)) >> + return false; >> + >> + /* Go through each access, compute corresponding rtx(regs or subregs) >> + for the expression. */ >> + int n = access_vec->length (); >> + int cur_access_index = 0; >> + for (; cur_access_index < n; cur_access_index++) >> + if (!scalarize_access ((*access_vec)[cur_access_index], regs)) >> + break; >> + >> + /* Avoid un-scalarized accesses. */ >> + if (cur_access_index != n) >> + { >> + base_access_vec->remove (base); >> + return false; >> + } >> + >> + /* Bind/map expr(tree) to scalarized rtx. */ >> + for (int j = 0; j < n; j++) >> + { >> + access_p access = (*access_vec)[j]; >> + expr_rtx_vec->put (access->expr, access->rtx_val); > > why do we compute RTL for each access? We only want to change > the representation of the decl - how RTL expansion "scalarizes" > an aggregate. It does this by assigning it a mode which we want > to change. Changing the representation of the decl (e.g. set DECL_RTL as parallel with registers) could handle most cases. There may be one issue with partial scalarization. (which is not supported by this patch either.) e.g. "struct A { int i,j; long d;}" and insns: "_3=arg.i+arg.j; foo (&arg.d);" arg.i and arg.j may be accessed through register directly, but arg.d may need to be handled in memory. To support this kind of case, we may need a way to tell which member of 'arg' can (or can not) be accessed through the registers. This is the reason why I'm trying to bind an RTL to a TREE expression(not only DECL TREE): If a TREE expr has the registers RTL then uses it, otherwise, the expr would be in the stack. > > If we want to really dis-aggregate the aggregate in the IL then > I believe we should do this on the GIMPLE level - which means > having a way to tie the "default definitions" to the incoming RTL. > Which I think we have. Yes, most aggregates have been scalarized(dis-aggregate) by tree-sra/ipa-sra on the GIMPLE level. In expander, there are some special cases that need to be handled: - aggregate parameters/returns passing by registers, but combined as INT mode: e.g. "struct A{float a; float b}" may has DImode. - aggregate parameters/returns passing by registers, but with BLKmode and stored to stack even if it is safe to use dis-aggregate. > > foo (struct X x) > { > <bb2>: > x_a_1 = x.a; > x_b_2 = x.b; > x_c_3 = x.c; > ... > > } > > Another possibility would be to have the parameters be SSA > names when the parameter is not (fully) passed in memory > and have the stack materialization explicit as aggregate > copy. > > I realize the complication is the way we transfer the incoming > hard registers to pseudos - and I think we simply need to > extend what DECL_RTL a PARM_DECL can have for the case we are > after - allow sth like (vec:TI [(reg:SI ..) (reg:SI ..) ... ]) > or so and have access expansion handle that in addition to > the already supported (concat:..). There's some support for > (parallel ...), at least for returns, but it might be too > sparse. Yes, it is complicated to handle various expressions (e.g. extract/store_bit_fields) on "vec:TI, concat:, parallel:". > > What I'm missing is (or I didn't find it) is code creating > accesses for the actual incoming / outgoing hardregister parts. This part is handled. It is where "set_scalar_rtx_for_aggregate_access" is called. e.g. in "assign_parm_setup_block". But, this patch directly uses the hard registers from incoming/outgoing which is computed by existing code, e.g. ---------- set_decl_incoming_rtl (parm, data.entry_parm, false); + rtx incoming = DECL_INCOMING_RTL (parm); + if (GET_CODE (incoming) == PARALLEL) + set_scalar_rtx_for_aggregate_access (parm, incoming); > Where's that taken into account? I also don't see any handling > of overlapping accesses - that is, when the choice for the This patch does not check overlapping access. One reason: this patch supports "reading" from parameter only. So it is safe to use the registers/pseudo for reading expression, even there are overlapping accesses. (Yes, for some cases, it is also safe to use reg for writing. But, this patch need to be enhanced carefully.) > pieces isn't obvious? In fact I can't see _any_ code that > tries to compute the set of scalar replacements? Consider > > struct X { int a; int b; }; > > long foo (struct X x) > { > long y; > memcpy (&y, &x, sizeof (long)); > return y + x.a + x.b; > } > > you'll get a 8 byte read and two 4 byte reads. What's going > to be your choice for decomposing 'X'? On x86 it will have > DImode, passed in %rdi so decomposing it will not improve > code generation. Since there is a call that takes the address of the parameter, So, the current patch uses the stack for the parameter but does not use the incoming registers. > > struct X { int a; int b; int c; int d; }; > > long foo (struct X x) > { > long y; > memcpy (&y, &x, sizeof (long)); > return y + x.c + x.d; > } > > on x86 we have TImode for X, but it's passed in two DImode > registers. For the first half you have an 8 byte access, > for the second half you have two 4 byte accesses. I'd > expect to decompose this to two DImode halves, based on > the incoming register layout and roughly matching > accesses. Assume there is no addr-taken, memcpy is sth like "y=x" (via union). Then, there are three accesses for "x": (offset=0, size=8), (offset=8,size=4), (12,4). Then three rtx is generated: scalar1=%rx:DI, scalar2=%ry:DI#0, scalar3=(%ry>>32)#0 > > struct X { int a; int b; int c; int d; }; > > long foo (struct X x) > { > long y; > memcpy (&y, &x.b, sizeof (long)); > return y + x.a + x.d; > } > > two DImode halves for the pseudos doens't make much sense > in this case - we have 'y' overlapping both. RTL opts > recover nicely in all the above cases when using TImode > so it's going to be important to not be overly aggressive. > > That IMHO also means when we need to do more fancy things > like rewriting accesses (thus when RTL expansion cannot > cope with our idea of how the base object expands) we should > think of dealing with this on the GIMPLE level. Thanks, I try to understand this. :) > > For power you seem to have the situation of BLKmode aggregates > that are passed in registers? I don't think we have this > situation on x86 for example. The mode of an aggregate may be DI/TI, and also BLK: If the size of the aggregate is not greater than the biggest INT, then it would use INT mode, otherwise, BLK would be used. If I remember correctly, x86 may also be similar. > > That said - I'm not sure I reverse engineered what you do > correctly. A more thorough description with the problem > you are solving plus how you attack it from a high level > perspective would have been useful. Thanks for pointing out this! And thanks so much for your time on this review! BR, Jeff (Jiufu Guo) > > Thanks, > Richard. > >> + } >> + >> + return true; >> +} >> + >> +static expand_sra *current_sra = NULL; >> + >> +/* Check If there is an sra access for the expr. >> + Return the correspond scalar sym for the access. */ >> + >> +rtx >> +get_scalar_rtx_for_aggregate_expr (tree expr) >> +{ >> + return current_sra ? current_sra->get_scalarized_rtx (expr) : NULL_RTX; >> +} >> + >> +/* Compute/Set RTX registers for those accesses on BASE. */ >> + >> +void >> +set_scalar_rtx_for_aggregate_access (tree base, rtx regs) >> +{ >> + if (!current_sra) >> + return; >> + current_sra->scalarize_accesses (base, regs); >> +} >> + >> +void >> +set_scalar_rtx_for_returns () >> +{ >> + if (!current_sra) >> + return; >> + >> + tree res = DECL_RESULT (current_function_decl); >> + gcc_assert (res); >> + edge_iterator ei; >> + edge e; >> + FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds) >> + if (greturn *r = safe_dyn_cast<greturn *> (*gsi_last_bb (e->src))) >> + { >> + tree val = gimple_return_retval (r); >> + if (val && VAR_P (val)) >> + current_sra->scalarize_accesses (val, DECL_RTL (res)); >> + } >> +} >> + >> /* Return an expression tree corresponding to the RHS of GIMPLE >> statement STMT. */ >> >> @@ -3778,7 +4241,8 @@ expand_return (tree retval) >> >> /* If we are returning the RESULT_DECL, then the value has already >> been stored into it, so we don't have to do anything special. */ >> - if (TREE_CODE (retval_rhs) == RESULT_DECL) >> + if (TREE_CODE (retval_rhs) == RESULT_DECL >> + || get_scalar_rtx_for_aggregate_expr (retval_rhs)) >> expand_value_return (result_rtl); >> >> /* If the result is an aggregate that is being returned in one (or more) >> @@ -4422,6 +4886,9 @@ expand_debug_expr (tree exp) >> int unsignedp = TYPE_UNSIGNED (TREE_TYPE (exp)); >> addr_space_t as; >> scalar_int_mode op0_mode, op1_mode, addr_mode; >> + rtx x = get_scalar_rtx_for_aggregate_expr (exp); >> + if (x) >> + return NULL_RTX;/* optimized out. */ >> >> switch (TREE_CODE_CLASS (TREE_CODE (exp))) >> { >> @@ -6624,6 +7091,9 @@ pass_expand::execute (function *fun) >> auto_bitmap forced_stack_vars; >> discover_nonconstant_array_refs (forced_stack_vars); >> >> + current_sra = new expand_sra; >> + scan_function (cfun, *current_sra); >> + >> /* Make sure all values used by the optimization passes have sane >> defaults. */ >> reg_renumber = 0; >> @@ -7052,6 +7522,8 @@ pass_expand::execute (function *fun) >> loop_optimizer_finalize (); >> } >> >> + delete current_sra; >> + current_sra = NULL; >> timevar_pop (TV_POST_EXPAND); >> >> return 0; >> diff --git a/gcc/expr.cc b/gcc/expr.cc >> index >> 174f8acb269ab5450fc799516471d5a2bd9b9efa..57b037040d6d8e8c98b2befcb556221c0c5604c4 >> 100644 >> --- a/gcc/expr.cc >> +++ b/gcc/expr.cc >> @@ -100,6 +100,7 @@ static void do_tablejump (rtx, machine_mode, rtx, rtx, >> rtx, >> static rtx const_vector_from_tree (tree); >> static tree tree_expr_size (const_tree); >> static void convert_mode_scalar (rtx, rtx, int); >> +rtx get_scalar_rtx_for_aggregate_expr (tree); >> >> >> /* This is run to set up which modes can be used >> @@ -5618,11 +5619,12 @@ expand_assignment (tree to, tree from, bool >> nontemporal) >> Assignment of an array element at a constant index, and assignment of >> an array element in an unaligned packed structure field, has the same >> problem. Same for (partially) storing into a non-memory object. */ >> - if (handled_component_p (to) >> - || (TREE_CODE (to) == MEM_REF >> - && (REF_REVERSE_STORAGE_ORDER (to) >> - || mem_ref_refers_to_non_mem_p (to))) >> - || TREE_CODE (TREE_TYPE (to)) == ARRAY_TYPE) >> + if (!get_scalar_rtx_for_aggregate_expr (to) >> + && (handled_component_p (to) >> + || (TREE_CODE (to) == MEM_REF >> + && (REF_REVERSE_STORAGE_ORDER (to) >> + || mem_ref_refers_to_non_mem_p (to))) >> + || TREE_CODE (TREE_TYPE (to)) == ARRAY_TYPE)) >> { >> machine_mode mode1; >> poly_int64 bitsize, bitpos; >> @@ -8912,6 +8914,20 @@ expand_constructor (tree exp, rtx target, enum >> expand_modifier modifier, >> && ! mostly_zeros_p (exp)) >> return NULL_RTX; >> >> + if (target && GET_CODE (target) == PARALLEL && all_zeros_p (exp)) >> + { >> + int length = XVECLEN (target, 0); >> + int start = XEXP (XVECEXP (target, 0, 0), 0) ? 0 : 1; >> + for (int i = start; i < length; i++) >> + { >> + rtx dst = XEXP (XVECEXP (target, 0, i), 0); >> + rtx zero = CONST0_RTX (GET_MODE (dst)); >> + gcc_assert (zero); >> + emit_move_insn (dst, zero); >> + } >> + return target; >> + } >> + >> /* Handle calls that pass values in multiple non-contiguous >> locations. The Irix 6 ABI has examples of this. */ >> if (target == 0 || ! safe_from_p (target, exp, 1) >> @@ -9006,6 +9022,9 @@ expand_expr_real (tree exp, rtx target, machine_mode >> tmode, >> ret = CONST0_RTX (tmode); >> return ret ? ret : const0_rtx; >> } >> + rtx x = get_scalar_rtx_for_aggregate_expr (exp); >> + if (x) >> + return x; >> >> ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl, >> inner_reference_p); >> diff --git a/gcc/function.cc b/gcc/function.cc >> index >> dd2c1136e0725f55673f28e0eeaf4c91ad18e93f..7fe927bd36beac11466ca9fca12800892b57f0be >> 100644 >> --- a/gcc/function.cc >> +++ b/gcc/function.cc >> @@ -2740,6 +2740,9 @@ assign_parm_find_stack_rtl (tree parm, struct >> assign_parm_data_one *data) >> data->stack_parm = stack_parm; >> } >> >> +extern void set_scalar_rtx_for_aggregate_access (tree, rtx); >> +extern void set_scalar_rtx_for_returns (); >> + >> /* A subroutine of assign_parms. Adjust DATA->ENTRY_RTL such that it's >> always valid and contiguous. */ >> >> @@ -3115,8 +3118,24 @@ assign_parm_setup_block (struct assign_parm_data_all >> *all, >> emit_move_insn (mem, entry_parm); >> } >> else >> - move_block_from_reg (REGNO (entry_parm), mem, >> - size_stored / UNITS_PER_WORD); >> + { >> + int regno = REGNO (entry_parm); >> + int nregs = size_stored / UNITS_PER_WORD; >> + move_block_from_reg (regno, mem, nregs); >> + >> + rtx *tmps = XALLOCAVEC (rtx, nregs); >> + machine_mode mode = word_mode; >> + HOST_WIDE_INT word_size = GET_MODE_SIZE (mode).to_constant (); >> + for (int i = 0; i < nregs; i++) >> + { >> + rtx reg = gen_rtx_REG (mode, regno + i); >> + rtx off = GEN_INT (word_size * i); >> + tmps[i] = gen_rtx_EXPR_LIST (VOIDmode, reg, off); >> + } >> + >> + rtx regs = gen_rtx_PARALLEL (BLKmode, gen_rtvec_v (nregs, tmps)); >> + set_scalar_rtx_for_aggregate_access (parm, regs); >> + } >> } >> else if (data->stack_parm == 0 && !TYPE_EMPTY_P (data->arg.type)) >> { >> @@ -3716,6 +3735,10 @@ assign_parms (tree fndecl) >> else >> set_decl_incoming_rtl (parm, data.entry_parm, false); >> >> + rtx incoming = DECL_INCOMING_RTL (parm); >> + if (GET_CODE (incoming) == PARALLEL) >> + set_scalar_rtx_for_aggregate_access (parm, incoming); >> + >> assign_parm_adjust_stack_rtl (&data); >> >> if (assign_parm_setup_block_p (&data)) >> @@ -5136,6 +5159,7 @@ expand_function_start (tree subr) >> { >> gcc_assert (GET_CODE (hard_reg) == PARALLEL); >> set_parm_rtl (res, gen_group_rtx (hard_reg)); >> + set_scalar_rtx_for_returns (); >> } >> } >> >> diff --git a/gcc/tree-sra.h b/gcc/tree-sra.h >> index >> f20266c46226f7840299a768cb575f6f92b54207..bd0396e672b30f7ef66c305d8d131e91639039d7 >> 100644 >> --- a/gcc/tree-sra.h >> +++ b/gcc/tree-sra.h >> @@ -19,6 +19,83 @@ You should have received a copy of the GNU General Public >> License >> along with GCC; see the file COPYING3. If not see >> <http://www.gnu.org/licenses/>. */ >> >> +struct base_access >> +{ >> + /* Values returned by get_ref_base_and_extent, indicates the >> + OFFSET, SIZE and BASE of the access. */ >> + HOST_WIDE_INT offset; >> + HOST_WIDE_INT size; >> + tree base; >> + >> + /* The context expression of this access. */ >> + tree expr; >> + >> + /* Indicates this is a write access. */ >> + bool write : 1; >> + >> + /* Indicates if this access is made in reverse storage order. */ >> + bool reverse : 1; >> +}; >> + >> +/* Default template for sra_scan_function. */ >> + >> +struct default_analyzer >> +{ >> + /* Template analyze functions. */ >> + void analyze_phi (gphi *){}; >> + void pre_analyze_stmt (gimple *){}; >> + void analyze_return (greturn *){}; >> + void analyze_assign (gassign *){}; >> + void analyze_call (gcall *){}; >> + void analyze_asm (gasm *){}; >> + void analyze_default_stmt (gimple *){}; >> +}; >> + >> +/* Scan function and look for interesting expressions. */ >> + >> +template <typename analyzer> >> +void >> +scan_function (struct function *fun, analyzer &a) >> +{ >> + basic_block bb; >> + FOR_EACH_BB_FN (bb, fun) >> + { >> + for (gphi_iterator gsi = gsi_start_phis (bb); !gsi_end_p (gsi); >> + gsi_next (&gsi)) >> + a.analyze_phi (gsi.phi ()); >> + >> + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); >> + gsi_next (&gsi)) >> + { >> + gimple *stmt = gsi_stmt (gsi); >> + a.pre_analyze_stmt (stmt); >> + >> + switch (gimple_code (stmt)) >> + { >> + case GIMPLE_RETURN: >> + a.analyze_return (as_a<greturn *> (stmt)); >> + break; >> + >> + case GIMPLE_ASSIGN: >> + a.analyze_assign (as_a<gassign *> (stmt)); >> + break; >> + >> + case GIMPLE_CALL: >> + a.analyze_call (as_a<gcall *> (stmt)); >> + break; >> + >> + case GIMPLE_ASM: >> + a.analyze_asm (as_a<gasm *> (stmt)); >> + break; >> + >> + default: >> + a.analyze_default_stmt (stmt); >> + break; >> + } >> + } >> + } >> +} >> + >> bool type_internals_preclude_sra_p (tree type, const char **msg); >> >> /* Return true iff TYPE is stdarg va_list type (which early SRA and IPA-SRA >> diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C >> b/gcc/testsuite/g++.target/powerpc/pr102024.C >> index >> 769585052b507ad971868795f861106230c976e3..c8995cae707bb6e2e849275b823d2ba14d24a966 >> 100644 >> --- a/gcc/testsuite/g++.target/powerpc/pr102024.C >> +++ b/gcc/testsuite/g++.target/powerpc/pr102024.C >> @@ -5,7 +5,7 @@ >> // Test that a zero-width bit field in an otherwise homogeneous aggregate >> // generates a psabi warning and passes arguments in GPRs. >> >> -// { dg-final { scan-assembler-times {\mstd\M} 4 } } >> +// { dg-final { scan-assembler-times {\mmtvsrd\M} 4 } } >> >> struct a_thing >> { >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr108073.c >> b/gcc/testsuite/gcc.target/powerpc/pr108073.c >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..7dd1a4a326a181e0f35c9418af20a9bebabdfe4b >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/pr108073.c >> @@ -0,0 +1,29 @@ >> +/* { dg-do run } */ >> +/* { dg-options "-O2 -save-temps" } */ >> + >> +typedef struct DF {double a[4]; short s1; short s2; short s3; short s4; } >> DF; >> +typedef struct SF {float a[4]; int i1; int i2; } SF; >> + >> +/* { dg-final { scan-assembler-times {\mmtvsrd\M} 3 {target { >> has_arch_ppc64 && has_arch_pwr8 } } } } */ >> +/* { dg-final { scan-assembler-not {\mlwz\M} {target { has_arch_ppc64 && >> has_arch_pwr8 } } } } */ >> +/* { dg-final { scan-assembler-not {\mlhz\M} {target { has_arch_ppc64 && >> has_arch_pwr8 } } } } */ >> +short __attribute__ ((noipa)) foo_hi (DF a, int flag){if (flag == 2)return >> a.s2+a.s3;return 0;} >> +int __attribute__ ((noipa)) foo_si (SF a, int flag){if (flag == 2)return >> a.i2+a.i1;return 0;} >> +double __attribute__ ((noipa)) foo_df (DF arg, int flag){if (flag == >> 2)return arg.a[3];else return 0.0;} >> +float __attribute__ ((noipa)) foo_sf (SF arg, int flag){if (flag == >> 2)return arg.a[2]; return 0;} >> +float __attribute__ ((noipa)) foo_sf1 (SF arg, int flag){if (flag == >> 2)return arg.a[1];return 0;} >> + >> +DF gdf = {{1.0,2.0,3.0,4.0}, 1, 2, 3, 4}; >> +SF gsf = {{1.0f,2.0f,3.0f,4.0f}, 1, 2}; >> + >> +int main() >> +{ >> + if (!(foo_hi (gdf, 2) == 5 && foo_si (gsf, 2) == 3 && foo_df (gdf, 2) == >> 4.0 >> + && foo_sf (gsf, 2) == 3.0 && foo_sf1 (gsf, 2) == 2.0)) >> + __builtin_abort (); >> + if (!(foo_hi (gdf, 1) == 0 && foo_si (gsf, 1) == 0 && foo_df (gdf, 1) == 0 >> + && foo_sf (gsf, 1) == 0 && foo_sf1 (gsf, 1) == 0)) >> + __builtin_abort (); >> + return 0; >> +} >> + >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr65421-1.c >> b/gcc/testsuite/gcc.target/powerpc/pr65421-1.c >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..4e1f87f7939cbf1423772023ee392fc5200b6708 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/pr65421-1.c >> @@ -0,0 +1,6 @@ >> +/* PR target/65421 */ >> +/* { dg-options "-O2" } */ >> + >> +typedef struct LARGE {double a[4]; int arr[32];} LARGE; >> +LARGE foo (LARGE a){return a;} >> +/* { dg-final { scan-assembler-times {\mmemcpy\M} 1 } } */ >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr65421-2.c >> b/gcc/testsuite/gcc.target/powerpc/pr65421-2.c >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..8a8e1a0e9962317ba2c0942af8891b3c51f4d3a4 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/pr65421-2.c >> @@ -0,0 +1,32 @@ >> +/* PR target/65421 */ >> +/* { dg-options "-O2" } */ >> +/* { dg-require-effective-target powerpc_elfv2 } */ >> +/* { dg-require-effective-target has_arch_ppc64 } */ >> + >> +typedef struct FLOATS >> +{ >> + double a[3]; >> +} FLOATS; >> + >> +/* 3 lfd after returns also optimized */ >> +/* FLOATS ret_arg_pt (FLOATS *a){return *a;} */ >> + >> +/* 3 stfd */ >> +void st_arg (FLOATS a, FLOATS *p) {*p = a;} >> +/* { dg-final { scan-assembler-times {\mstfd\M} 3 } } */ >> + >> +/* blr */ >> +FLOATS ret_arg (FLOATS a) {return a;} >> + >> +typedef struct MIX >> +{ >> + double a[2]; >> + long l; >> +} MIX; >> + >> +/* std 3 param regs to return slot */ >> +MIX ret_arg1 (MIX a) {return a;} >> +/* { dg-final { scan-assembler-times {\mstd\M} 3 } } */ >> + >> +/* count insns */ >> +/* { dg-final { scan-assembler-times {(?n)^\s+[a-z]} 9 } } */ >>
