Message ID | 4E16ADAB.4000409@gnu.org |
---|---|
State | New |
Headers | show |
Paolo Bonzini <bonzini@gnu.org> writes: > df-scan.c has this code to deal with group sets: > > /* It is legal to have a set destination be a parallel. */ > if (GET_CODE (dst) == PARALLEL) > { > int i; > > for (i = XVECLEN (dst, 0) - 1; i >= 0; i--) > { > rtx temp = XVECEXP (dst, 0, i); > if (GET_CODE (temp) == EXPR_LIST || GET_CODE (temp) == CLOBBER > || GET_CODE (temp) == SET) > df_def_record_1 (collection_rec, > temp, bb, insn_info, > GET_CODE (temp) == CLOBBER > ? flags | DF_REF_MUST_CLOBBER : flags); > } > return; > } > > It seems to me that the case of (set (parallel [(set ...)])) and (set > (parallel [(clobber ...)])) is bogus. I would like to simplify it to > the following: > > /* It is legal to have a set destination be a parallel. */ > if (GET_CODE (dst) == PARALLEL) > { > int i; > > for (i = XVECLEN (dst, 0) - 1; i >= 0; i--) > { > rtx temp = XVECEXP (dst, 0, i); > assert (GET_CODE (temp) == EXPR_LIST); > df_def_record_1 (collection_rec, temp, bb, insn_info, flags); > } > return; > } > > Does this make sense? Yeah, the docs seem pretty certain that expr_list is the only valid choice. The docs also say that the first expr_list can be null: If @var{lval} is a @code{parallel}, it is used to represent the case of a function returning a structure in multiple registers. Each element of the @code{parallel} is an @code{expr_list} whose first operand is a @code{reg} and whose second operand is a @code{const_int} representing the offset (in bytes) into the structure at which the data in that register corresponds. The first element may be null to indicate that the structure is also passed partly in memory. but I can't see any code to handle that. Am I missing something, or does the lack of a crash here mean that we can remove the last sentence? (It might have been added for symmetry with argument passing, where this sort of thing is needed. But if it isn't actually used or implemented for returns, it might be less confusing to remove it.) > See the attached patch for the overall thing I > was thinking of. > > Paolo > > * df-scan.c (df_def_record_1): Assert a parallel must contain > an EXPR_LIST at this point. Receive the LOC and move its > extraction... > (df_defs_record): ... here. Remove superfluous braces. Looks good. Richard
On 07/08/2011 12:43 PM, Richard Sandiford wrote: > The docs also say that the first expr_list can be null: > > If @var{lval} is a @code{parallel}, it is used to represent the case of > a function returning a structure in multiple registers. Each element > of the @code{parallel} is an @code{expr_list} whose first operand is a > @code{reg} and whose second operand is a @code{const_int} representing the > offset (in bytes) into the structure at which the data in that register > corresponds. The first element may be null to indicate that the structure > is also passed partly in memory. > > but I can't see any code to handle that. Am I missing something, > or does the lack of a crash here mean that we can remove the last > sentence? > > (It might have been added for symmetry with argument passing, where this > sort of thing is needed. But if it isn't actually used or implemented for > returns, it might be less confusing to remove it.) Indeed. Dimitrios, can you pick up the patch since it will somewhat simplify your work to eliminate defs_generated? Paolo
On Fri, 8 Jul 2011, Paolo Bonzini wrote: > On 07/08/2011 12:43 PM, Richard Sandiford wrote: >> The docs also say that the first expr_list can be null: >> >> If @var{lval} is a @code{parallel}, it is used to represent the case of >> a function returning a structure in multiple registers. Each element >> of the @code{parallel} is an @code{expr_list} whose first operand is a >> @code{reg} and whose second operand is a @code{const_int} representing >> the >> offset (in bytes) into the structure at which the data in that register >> corresponds. The first element may be null to indicate that the >> structure >> is also passed partly in memory. >> >> but I can't see any code to handle that. Am I missing something, >> or does the lack of a crash here mean that we can remove the last >> sentence? >> >> (It might have been added for symmetry with argument passing, where this >> sort of thing is needed. But if it isn't actually used or implemented for >> returns, it might be less confusing to remove it.) > > Indeed. Dimitrios, can you pick up the patch since it will somewhat simplify > your work to eliminate defs_generated? I'll certainly try :-) Paolo, something else, in df_mark_reg() is it ever possible for regno to be >= FIRST_PSEUDO_REGISTER? An assert I've put doesn't trigger for my simple test :-) Thanks, Dimitris
On 07/08/2011 03:05 PM, Dimitrios Apostolou wrote: > > Paolo, something else, in df_mark_reg() is it ever possible for regno to > be >= FIRST_PSEUDO_REGISTER? An assert I've put doesn't trigger for my > simple test :-) From reading the docs of EH_RETURN_STACKADJ_RTX and EH_RETURN_HANDLER_RTX, it seems you're safe. This in df-problems.c also suggests the same: if (bb_index == EXIT_BLOCK) { unsigned regno; bitmap_iterator bi; EXECUTE_IF_SET_IN_BITMAP (df->exit_block_uses, FIRST_PSEUDO_REGISTER, regno, bi) gcc_unreachable (); } A more solid reasoning is that a pseudo cannot be considered live at exit or at entry to a function, because the caller would not know where it lives. That said, changing exit_block_uses and entry_block_defs to HARD_REG_SET would be a nice cleanup, but it would also touch target code due to targetm.extra_live_on_entry (entry_block_defs); I wouldn't bother for now until you're a bit more experienced. Unlike invalidated_by_call it shouldn't show up in profiles, or does it? Paolo
Thanks Paolo for the detailed explanation! On Fri, 8 Jul 2011, Paolo Bonzini wrote: > > That said, changing exit_block_uses and entry_block_defs to HARD_REG_SET would > be a nice cleanup, but it would also touch target code due to > > targetm.extra_live_on_entry (entry_block_defs); > I've already done that :-p > I wouldn't bother for now until you're a bit more experienced. Unlike > invalidated_by_call it shouldn't show up in profiles, or does it? Indeed it doesn't show, I just wanted to do it as a clean-up for transitioning to HARD_REG_SET all relevant sets in struct df_d. The only problem remaining is I need a bitmap_copy_from_hard_reg_set() function for df_lr_local_compute(), where the bb_info->use bitmap is initialised from the exit_block_uses HARD_REG_SET. Dimitris
* df-scan.c (df_def_record_1): Assert a parallel must contain an EXPR_LIST at this point. Receive the LOC and move its extraction... (df_defs_record): ... here. Remove superfluous braces. Index: df-scan.c =================================================================== --- df-scan.c (revision 169877) +++ df-scan.c (working copy) @@ -111,7 +111,7 @@ static void df_ref_record (enum df_ref_c rtx, rtx *, basic_block, struct df_insn_info *, enum df_ref_type, int ref_flags); -static void df_def_record_1 (struct df_collection_rec *, rtx, +static void df_def_record_1 (struct df_collection_rec *, rtx *, basic_block, struct df_insn_info *, int ref_flags); static void df_defs_record (struct df_collection_rec *, rtx, @@ -2922,19 +2922,10 @@ df_read_modify_subreg_p (rtx x) static void df_def_record_1 (struct df_collection_rec *collection_rec, - rtx x, basic_block bb, struct df_insn_info *insn_info, + rtx *loc, basic_block bb, struct df_insn_info *insn_info, int flags) { - rtx *loc; - rtx dst; - - /* We may recursively call ourselves on EXPR_LIST when dealing with PARALLEL - construct. */ - if (GET_CODE (x) == EXPR_LIST || GET_CODE (x) == CLOBBER) - loc = &XEXP (x, 0); - else - loc = &SET_DEST (x); - dst = *loc; + rtx dst = *loc; /* It is legal to have a set destination be a parallel. */ if (GET_CODE (dst) == PARALLEL) @@ -2944,12 +2935,9 @@ df_def_record_1 (struct df_collection_re for (i = XVECLEN (dst, 0) - 1; i >= 0; i--) { rtx temp = XVECEXP (dst, 0, i); - if (GET_CODE (temp) == EXPR_LIST || GET_CODE (temp) == CLOBBER - || GET_CODE (temp) == SET) - df_def_record_1 (collection_rec, - temp, bb, insn_info, - GET_CODE (temp) == CLOBBER - ? flags | DF_REF_MUST_CLOBBER : flags); + gcc_assert (GET_CODE (temp) == EXPR_LIST); + df_def_record_1 (collection_rec, &XEXP (temp, 0), + bb, insn_info, flags); } return; } @@ -3003,18 +2991,16 @@ df_defs_record (struct df_collection_rec { RTX_CODE code = GET_CODE (x); - if (code == SET || code == CLOBBER) - { - /* Mark the single def within the pattern. */ - int clobber_flags = flags; - clobber_flags |= (code == CLOBBER) ? DF_REF_MUST_CLOBBER : 0; - df_def_record_1 (collection_rec, x, bb, insn_info, clobber_flags); - } + if (code == SET) + df_def_record_1 (collection_rec, &SET_DEST (x), bb, insn_info, flags); + else if (code == CLOBBER) + { + flags |= DF_REF_MUST_CLOBBER; + df_def_record_1 (collection_rec, &XEXP (x, 0), bb, insn_info, flags); + } else if (code == COND_EXEC) - { - df_defs_record (collection_rec, COND_EXEC_CODE (x), - bb, insn_info, DF_REF_CONDITIONAL); - } + df_defs_record (collection_rec, COND_EXEC_CODE (x), + bb, insn_info, DF_REF_CONDITIONAL); else if (code == PARALLEL) { int i;