| Message ID | mpt1rznm4u5.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Tweak IRA handling of tying and earlyclobbers | expand |
On 2019-06-21 9:43 a.m., Richard Sandiford wrote: > make_early_clobber_and_input_conflicts records allocno conflicts > between inputs and earlyclobber outputs. It (rightly) avoids > doing this for inputs that are explicitly allowed to match the > output due to matching constraints. > > The problem is that whether this matching is allowed varies > between alternatives. At the moment the code avoids adding > a clobber if *any* enabled alternative allows the match, > even if some other operand makes that alternative impossible. > > The specific instance of this for SVE is that some alternatives > allow matched earlyclobbers when a third operand X is constant zero. > We should avoid adding conflicts when X really is constant zero, > but should ignore the match if X is nonzero or nonconstant. > > ira_setup_alts can already filter these alternatives out for us, > so all we need to do is use it in process_bb_node_lives. The > preferred_alternatives variable is only used for this earlyclobber > detection, so no other check should be affected. > > With the previous patch to check the reject weight in ira_setup_alts, > this has the effect of ignoring expensive alternatives if we have > other valid alternatives with zero cost. It seems reasonable to base > the heuristic on only the alternatives that we'd actually like to use, > but if this ends up being too aggressive, we could instead make the new > reject behaviour conditional and only use it for add_insn_allocno_copies. > This patch definitely improves the heuristics. The only missed part is a comment for preferred_alternatives /* The value of get_preferred_alternatives for the current instruction, supplemental to recog_data. */ static alternative_mask preferred_alternatives; The comment becomes misleading after the patch. With changing the comment, the patch is ok for me. Richard, thank you for the patches improving the RA. > 2019-06-21 Richard Sandiford <richard.sandiford@arm.com> > > gcc/ > * ira-lives.c (process_bb_node_lives): Use ira_setup_alts. > > Index: gcc/ira-lives.c > =================================================================== > --- gcc/ira-lives.c 2019-05-29 10:49:39.512701998 +0100 > +++ gcc/ira-lives.c 2019-06-21 14:34:19.071605402 +0100 > @@ -1236,9 +1236,7 @@ process_bb_node_lives (ira_loop_tree_nod > } > } > > - extract_insn (insn); > - preferred_alternatives = get_preferred_alternatives (insn); > - preprocess_constraints (insn); > + preferred_alternatives = ira_setup_alts (insn); > process_single_reg_class_operands (false, freq); > > if (call_p)
Vladimir Makarov <vmakarov@redhat.com> writes: > On 2019-06-21 9:43 a.m., Richard Sandiford wrote: >> make_early_clobber_and_input_conflicts records allocno conflicts >> between inputs and earlyclobber outputs. It (rightly) avoids >> doing this for inputs that are explicitly allowed to match the >> output due to matching constraints. >> >> The problem is that whether this matching is allowed varies >> between alternatives. At the moment the code avoids adding >> a clobber if *any* enabled alternative allows the match, >> even if some other operand makes that alternative impossible. >> >> The specific instance of this for SVE is that some alternatives >> allow matched earlyclobbers when a third operand X is constant zero. >> We should avoid adding conflicts when X really is constant zero, >> but should ignore the match if X is nonzero or nonconstant. >> >> ira_setup_alts can already filter these alternatives out for us, >> so all we need to do is use it in process_bb_node_lives. The >> preferred_alternatives variable is only used for this earlyclobber >> detection, so no other check should be affected. >> >> With the previous patch to check the reject weight in ira_setup_alts, >> this has the effect of ignoring expensive alternatives if we have >> other valid alternatives with zero cost. It seems reasonable to base >> the heuristic on only the alternatives that we'd actually like to use, >> but if this ends up being too aggressive, we could instead make the new >> reject behaviour conditional and only use it for add_insn_allocno_copies. >> > This patch definitely improves the heuristics. > > The only missed part is a comment for preferred_alternatives > > /* The value of get_preferred_alternatives for the current instruction, > supplemental to recog_data. */ > static alternative_mask preferred_alternatives; > > The comment becomes misleading after the patch. Oops, thanks for noticing that. > With changing the comment, the patch is ok for me. Thanks, here's what I installed after testing on aarch64-linux-gnu and x86_64-linux-gnu. Richard 2019-07-01 Richard Sandiford <richard.sandiford@arm.com> gcc/ * ira-lives.c (process_bb_node_lives): Use ira_setup_alts. Index: gcc/ira-lives.c =================================================================== --- gcc/ira-lives.c 2019-06-29 17:20:49.000000000 +0100 +++ gcc/ira-lives.c 2019-07-01 09:33:14.026477923 +0100 @@ -80,8 +80,9 @@ Software Foundation; either version 3, o /* The number of last call at which given allocno was saved. */ static int *allocno_saved_at_call; -/* The value of get_preferred_alternatives for the current instruction, - supplemental to recog_data. */ +/* The value returned by ira_setup_alts for the current instruction; + i.e. the set of alternatives that we should consider to be likely + candidates during reloading. */ static alternative_mask preferred_alternatives; /* If non-NULL, the source operand of a register to register copy for which @@ -1236,9 +1237,7 @@ process_bb_node_lives (ira_loop_tree_nod } } - extract_insn (insn); - preferred_alternatives = get_preferred_alternatives (insn); - preprocess_constraints (insn); + preferred_alternatives = ira_setup_alts (insn); process_single_reg_class_operands (false, freq); if (call_p)
Index: gcc/ira-lives.c =================================================================== --- gcc/ira-lives.c 2019-05-29 10:49:39.512701998 +0100 +++ gcc/ira-lives.c 2019-06-21 14:34:19.071605402 +0100 @@ -1236,9 +1236,7 @@ process_bb_node_lives (ira_loop_tree_nod } } - extract_insn (insn); - preferred_alternatives = get_preferred_alternatives (insn); - preprocess_constraints (insn); + preferred_alternatives = ira_setup_alts (insn); process_single_reg_class_operands (false, freq); if (call_p)