[01/10] Fix IRA ICE.

Message ID f59df31d485d71c8e4c68c74ef1ca1dc4a9dd8d4.1542381960.git.ams@codesourcery.com
State New
Headers show
Series
  • AMD GCN Port v2
Related show

Commit Message

Andrew Stubbs Nov. 16, 2018, 4:27 p.m.
This patch is unchanged from that which was posted before.  Discussion
fizzled out there and I was too busy with other patches to restart it
then.  This issue needs to be resolved before libgfortran can be
compiled for GCN.

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:

..../libgfortran/generated/matmul_r8.c: In function 'matmul_r8':
..../libgfortran/generated/matmul_r8.c:3002:1: internal compiler error: in setup_preferred_alternate_classes_for_new_pseudos, at ira.c:2772

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-11-16  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

Jeff Law Nov. 21, 2018, 12:47 a.m. | #1
On 11/16/18 9:27 AM, Andrew Stubbs wrote:
> 

> This patch is unchanged from that which was posted before.  Discussion

> fizzled out there and I was too busy with other patches to restart it

> then.  This issue needs to be resolved before libgfortran can be

> compiled for GCN.

> 

> 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:

> 

> ..../libgfortran/generated/matmul_r8.c: In function 'matmul_r8':

> ..../libgfortran/generated/matmul_r8.c:3002:1: internal compiler error: in setup_preferred_alternate_classes_for_new_pseudos, at ira.c:2772

> 

> 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-11-16  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.

This seems like a really gross hack and sets an expectation that
generating registers in the target after IRA has started is OK.  It is
not OK.  THe fact that this works is, IMHO, likely an accident.

I think this comes back to the fundamental representational issue with
the EXEC handling that still needs to be addressed.

Jeff
Andrew Stubbs Nov. 21, 2018, 11:35 a.m. | #2
On 21/11/2018 00:47, Jeff Law wrote:
> This seems like a really gross hack and sets an expectation that

> generating registers in the target after IRA has started is OK.  It is

> not OK.  THe fact that this works is, IMHO, likely an accident.


What's the proper test for this? Neither lra_in_progress nor 
reload_in_progress is set here, and can_create_pseudos returns true.

The patterns have the ability to not generate registers, but they don't 
know not to.

Richard Sandiford has stated that it should be OK, but perhaps the other 
architectures also work by accident?

In fact, since we're using LRA (not reload), my understanding is that I 
ought to be able to create new pseudos right up until reload_completed. 
(Although, my experience is that it's easy to get into an infinite loop 
doing that.)

> I think this comes back to the fundamental representational issue with

> the EXEC handling that still needs to be addressed.


Undoubtedly, this makes it worse, but even without that I'd still want 
to expand vector memory moves long before split1, so at least some cases 
have to generate additional registers. (Perhaps IRA doesn't create 
memory moves though? I'm not sure.)

I'm going to investigate how easy it is to fix the EXEC representation 
issues. I've been resisting because I had a deadline to make, and it's 
bound to be an invasive and destabilizing alteration (albeit largely 
mechanical), but if it's going to be a barrier to commit then probably 
it's become time. :-(

Andrew
Richard Sandiford Dec. 8, 2018, 12:14 p.m. | #3
Andrew Stubbs <ams@codesourcery.com> writes:
> On 21/11/2018 00:47, Jeff Law wrote:

>> This seems like a really gross hack and sets an expectation that

>> generating registers in the target after IRA has started is OK.  It is

>> not OK.  THe fact that this works is, IMHO, likely an accident.

>

> What's the proper test for this? Neither lra_in_progress nor 

> reload_in_progress is set here, and can_create_pseudos returns true.

>

> The patterns have the ability to not generate registers, but they don't 

> know not to.

>

> Richard Sandiford has stated that it should be OK, but perhaps the other 

> architectures also work by accident?


Yeah, my understanding was that targets could create new registers here,
and I thought targets did in some situations.  But that's also why I'm
suspicious of the patch.  If GCN is doing something valid and IRA isn't
coping, then the patch seems to be fixing the problem in the wrong place.

So I still agree with Jeff on the gross hack thing...

Richard

> In fact, since we're using LRA (not reload), my understanding is that I 

> ought to be able to create new pseudos right up until reload_completed. 

> (Although, my experience is that it's easy to get into an infinite loop 

> doing that.)

>

>> I think this comes back to the fundamental representational issue with

>> the EXEC handling that still needs to be addressed.

>

> Undoubtedly, this makes it worse, but even without that I'd still want 

> to expand vector memory moves long before split1, so at least some cases 

> have to generate additional registers. (Perhaps IRA doesn't create 

> memory moves though? I'm not sure.)

>

> I'm going to investigate how easy it is to fix the EXEC representation 

> issues. I've been resisting because I had a deadline to make, and it's 

> bound to be an invasive and destabilizing alteration (albeit largely 

> mechanical), but if it's going to be a barrier to commit then probably 

> it's become time. :-(

>

> Andrew
Jeff Law Dec. 8, 2018, 4:23 p.m. | #4
On 12/8/18 5:14 AM, Richard Sandiford wrote:
> Andrew Stubbs <ams@codesourcery.com> writes:

>> On 21/11/2018 00:47, Jeff Law wrote:

>>> This seems like a really gross hack and sets an expectation that

>>> generating registers in the target after IRA has started is OK.  It is

>>> not OK.  THe fact that this works is, IMHO, likely an accident.

>>

>> What's the proper test for this? Neither lra_in_progress nor 

>> reload_in_progress is set here, and can_create_pseudos returns true.

>>

>> The patterns have the ability to not generate registers, but they don't 

>> know not to.

>>

>> Richard Sandiford has stated that it should be OK, but perhaps the other 

>> architectures also work by accident?

> 

> Yeah, my understanding was that targets could create new registers here,

> and I thought targets did in some situations.  But that's also why I'm

> suspicious of the patch.  If GCN is doing something valid and IRA isn't

> coping, then the patch seems to be fixing the problem in the wrong place.

IRA/LRA can create new pseudos internally.  What I'm much less sure
about is whether or not the target can create them.  WHen IRA/LRA
creates one internally it has the chance to update the various internal
structures it needs.  That can't happen with a pseudo created by the
target this late.  Vlad would know for sure.

jeff
Andrew Stubbs Dec. 10, 2018, 3:21 p.m. | #5
On 08/12/2018 16:23, Jeff Law wrote:
> On 12/8/18 5:14 AM, Richard Sandiford wrote:

>> Andrew Stubbs <ams@codesourcery.com> writes:

>>> On 21/11/2018 00:47, Jeff Law wrote:

>>>> This seems like a really gross hack and sets an expectation that

>>>> generating registers in the target after IRA has started is OK.  It is

>>>> not OK.  THe fact that this works is, IMHO, likely an accident.

>>>

>>> What's the proper test for this? Neither lra_in_progress nor

>>> reload_in_progress is set here, and can_create_pseudos returns true.

>>>

>>> The patterns have the ability to not generate registers, but they don't

>>> know not to.

>>>

>>> Richard Sandiford has stated that it should be OK, but perhaps the other

>>> architectures also work by accident?

>>

>> Yeah, my understanding was that targets could create new registers here,

>> and I thought targets did in some situations.  But that's also why I'm

>> suspicious of the patch.  If GCN is doing something valid and IRA isn't

>> coping, then the patch seems to be fixing the problem in the wrong place.

> IRA/LRA can create new pseudos internally.  What I'm much less sure

> about is whether or not the target can create them.  WHen IRA/LRA

> creates one internally it has the chance to update the various internal

> structures it needs.  That can't happen with a pseudo created by the

> target this late.  Vlad would know for sure.


In any case, the new patch series that I will post soon does not appear 
to need this patch any more. (I'll post that as soon as I figure out an 
unrelated problem with jump threading.)

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