| Message ID | 54304bb66c95238afe5a603eff894caf56ac19ca.1536144068.git.ams@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | AMD GCN Port | expand |
<ams@codesourcery.com> writes: > The IRA pass makes an assumption that any pseudos created after the pass begins > were created explicitly by the pass itself and therefore will have > corresponding entries in its other tables. > > The GCN back-end, however, often creates additional pseudos, in expand > patterns, to represent the necessary EXEC value, and these break IRA's > assumption and cause ICEs. > > This patch simply has IRA skip unknown pseudos, and the problem goes away. > > Presumably, it's not ideal that these registers have not been processed by IRA, > but it does not appear to do any real harm. Could you go into more detail about how this happens? Other targets also create pseudos in their move patterns. Richard
On 17/09/18 10:22, Richard Sandiford wrote: > <ams@codesourcery.com> writes: >> The IRA pass makes an assumption that any pseudos created after the pass begins >> were created explicitly by the pass itself and therefore will have >> corresponding entries in its other tables. >> >> The GCN back-end, however, often creates additional pseudos, in expand >> patterns, to represent the necessary EXEC value, and these break IRA's >> assumption and cause ICEs. >> >> This patch simply has IRA skip unknown pseudos, and the problem goes away. >> >> Presumably, it's not ideal that these registers have not been processed by IRA, >> but it does not appear to do any real harm. > > Could you go into more detail about how this happens? Other targets > also create pseudos in their move patterns. Here's a simplified snippet from the machine description: (define_expand "mov<mode>" [(set (match_operand:VEC_REG_MODE 0 "nonimmediate_operand") (match_operand:VEC_REG_MODE 1 "general_operand"))] "" { [...] if (can_create_pseudo_p ()) { rtx exec = gcn_full_exec_reg (); rtx undef = gcn_gen_undef (<MODE>mode); [...] emit_insn (gen_mov<mode>_vector (operands[0], operands[1], exec undef)); [...] DONE; } }) gcn_full_exec_reg creates a new pseudo. It gets used as the mask parameter of a vec_merge. These registers then trip the asserts in ira.c. In the case of setup_preferred_alternate_classes_for_new_pseudos it's because they have numbers greater than "start" but have not been initialized with different ORIGINAL_REGNO (why would they have been?) In the case of move_unallocated_pseudos it's because the table pseudo_replaced_reg only has entries for the new pseudos directly created by find_moveable_pseudos, not the ones created indirectly. Andrew
Andrew Stubbs <ams@codesourcery.com> writes: > On 17/09/18 10:22, Richard Sandiford wrote: >> <ams@codesourcery.com> writes: >>> The IRA pass makes an assumption that any pseudos created after the >>> pass begins >>> were created explicitly by the pass itself and therefore will have >>> corresponding entries in its other tables. >>> >>> The GCN back-end, however, often creates additional pseudos, in expand >>> patterns, to represent the necessary EXEC value, and these break IRA's >>> assumption and cause ICEs. >>> >>> This patch simply has IRA skip unknown pseudos, and the problem goes away. >>> >>> Presumably, it's not ideal that these registers have not been >>> processed by IRA, >>> but it does not appear to do any real harm. >> >> Could you go into more detail about how this happens? Other targets >> also create pseudos in their move patterns. > > Here's a simplified snippet from the machine description: > > (define_expand "mov<mode>" > > > > [(set (match_operand:VEC_REG_MODE 0 "nonimmediate_operand") > > > > (match_operand:VEC_REG_MODE 1 "general_operand"))] > > > > "" > > > > { > > > > [...] > > > > > if (can_create_pseudo_p ()) > > > > { > > > > rtx exec = gcn_full_exec_reg (); > rtx undef = gcn_gen_undef (<MODE>mode); > > > [...] > > emit_insn (gen_mov<mode>_vector (operands[0], operands[1], exec > undef)); > [...] > > DONE; > } > }) > > gcn_full_exec_reg creates a new pseudo. It gets used as the mask > parameter of a vec_merge. > > These registers then trip the asserts in ira.c. > > In the case of setup_preferred_alternate_classes_for_new_pseudos it's > because they have numbers greater than "start" but have not been > initialized with different ORIGINAL_REGNO (why would they have been?) > > In the case of move_unallocated_pseudos it's because the table > pseudo_replaced_reg only has entries for the new pseudos directly > created by find_moveable_pseudos, not the ones created indirectly. What I more meant was: where do the moves that introduce the new pseudos get created? Almost all targets' move patterns introduce new pseudos if can_create_pseudo_p in certain circumstances, so GCN isn't doing anything unusual in the outline above. I think it comes down to the specifics of which kinds of operands require these temporaries and where the moves are being introduced. AIUI IRA normally calls expand_reg_info () at a suitable point to cope with new pseudos. It sounds like we might be missing a call somewhere. Richard
On 20/09/18 13:46, Richard Sandiford wrote: > Andrew Stubbs <ams@codesourcery.com> writes: >> In the case of move_unallocated_pseudos it's because the table >> pseudo_replaced_reg only has entries for the new pseudos directly >> created by find_moveable_pseudos, not the ones created indirectly. > > What I more meant was: where do the moves that introduce the new > pseudos get created? For find_moveable_pseudos, I believe it's where it calls gen_move_insn. > Almost all targets' move patterns introduce new pseudos if > can_create_pseudo_p in certain circumstances, so GCN isn't doing > anything unusual in the outline above. I think it comes down to > the specifics of which kinds of operands require these temporaries > and where the moves are being introduced. GCN creates new pseudos for all vector moves. Maybe that's just less exotic than other targets do? > AIUI IRA normally calls expand_reg_info () at a suitable point > to cope with new pseudos. It sounds like we might be missing > a call somewhere. Yes, it does, but one of the places I had to patch is *within* expand_reg_info: it's setup_preferred_alternate_classes_for_new_pseudos that asserts for pseudos created by gen_move_insn. Andrew
diff --git a/gcc/ira.c b/gcc/ira.c index def194a..e0c293c 100644 --- a/gcc/ira.c +++ b/gcc/ira.c @@ -2769,7 +2769,12 @@ setup_preferred_alternate_classes_for_new_pseudos (int start) for (i = start; i < max_regno; i++) { old_regno = ORIGINAL_REGNO (regno_reg_rtx[i]); - ira_assert (i != old_regno); + + /* Skip any new pseudos not created directly by this pass. + gen_move_insn can do this on AMD GCN, for example. */ + if (i == old_regno) + continue; + setup_reg_classes (i, reg_preferred_class (old_regno), reg_alternate_class (old_regno), reg_allocno_class (old_regno)); @@ -5054,6 +5059,12 @@ move_unallocated_pseudos (void) { int idx = i - first_moveable_pseudo; rtx other_reg = pseudo_replaced_reg[idx]; + + /* Skip any new pseudos not created directly by find_moveable_pseudos. + gen_move_insn can do this on AMD GCN, for example. */ + if (!other_reg) + continue; + rtx_insn *def_insn = DF_REF_INSN (DF_REG_DEF_CHAIN (i)); /* The use must follow all definitions of OTHER_REG, so we can insert the new definition immediately after any of them. */