[16/25] Fix IRA ICE.

Message ID 54304bb66c95238afe5a603eff894caf56ac19ca.1536144068.git.ams@codesourcery.com
State Superseded
Headers show
Series
  • AMD GCN Port
Related show

Commit Message

Andrew Stubbs Sept. 5, 2018, 11:51 a.m.
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.

2018-09-05  Andrew Stubbs  <ams@codesourcery.com>

	gcc/
	* ira.c (setup_preferred_alternate_classes_for_new_pseudos): Skip
	pseudos not created by this pass.
	(move_unallocated_pseudos): Likewise.
---
 gcc/ira.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Richard Sandiford Sept. 17, 2018, 9:22 a.m. | #1
<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
Andrew Stubbs Sept. 18, 2018, 9:23 p.m. | #2
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
Richard Sandiford Sept. 20, 2018, 12:46 p.m. | #3
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
Andrew Stubbs Sept. 20, 2018, 1:33 p.m. | #4
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

Patch

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.  */