[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[amodra at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #17 from Alan Modra  ---
*** Bug 68959 has been marked as a duplicate of this bug. ***

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[meissner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #16 from Michael Meissner  ---
Author: meissner
Date: Thu Feb  4 00:39:34 2016
New Revision: 233120

URL: https://gcc.gnu.org/viewcvs?rev=233120=gcc=rev
Log:
2016-02-03  Michael Meissner  
Vladimir Makarov  

PR target/69461
* config/rs6000/rs6000.c (rs6000_legitimate_address_p): Fix thinko
in validating fused toc addresses.


Modified:
trunk/gcc/ChangeLog
trunk/gcc/config/rs6000/rs6000.c

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[vmakarov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #13 from Vladimir Makarov  ---
Author: vmakarov
Date: Wed Feb  3 17:58:34 2016
New Revision: 233107

URL: https://gcc.gnu.org/viewcvs?rev=233107=gcc=rev
Log:
2016-02-03  Vladimir Makarov  
Alexandre Oliva  

PR target/69461
* lra-constraints.c (simplify_operand_subreg): Check additionally
address validity after potential reloading.
(process_address_1): Check insns validity.  In case of failure do
nothing.

2016-02-03  Vladimir Makarov  
Alexandre Oliva  

PR target/69461
* gcc.target/powerpc/pr69461.c: New.


Added:
trunk/gcc/testsuite/gcc.target/powerpc/pr69461.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/lra-constraints.c
trunk/gcc/testsuite/ChangeLog

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[vmakarov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #15 from Vladimir Makarov  ---
(In reply to Alexandre Oliva from comment #12)
> Vlad, thanks for taking this over.  Let me just point out, just in case you
> missed, that I believe it is important for any register allocator to test
> HARD_REGNO_MODE_OK, and that AFAICT we were not doing that.  Please let me
> know if I'm mistaken.  Thanks,


No, you are not mistaken.  But the problem is that not always all hard
registers of a class can have HARD_REGNO_MODE_OK the same value.  As we don't
know yet the exact hard reg at some point of LRA (which is different from
reload) and can not guarantee the pseudo will get a specific hard register
later.

Actually, there are pros and cons for each solution.

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[trippels at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

Markus Trippelsdorf  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #14 from Markus Trippelsdorf  ---
Fixed. Thanks.
Firefox now builds fine with -mlra (and -flto) on gcc112.

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[aoliva at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

Alexandre Oliva  changed:

   What|Removed |Added

   Assignee|aoliva at gcc dot gnu.org  |vmakarov at gcc dot 
gnu.org

--- Comment #12 from Alexandre Oliva  ---
Vlad, thanks for taking this over.  Let me just point out, just in case you
missed, that I believe it is important for any register allocator to test
HARD_REGNO_MODE_OK, and that AFAICT we were not doing that.  Please let me know
if I'm mistaken.  Thanks,

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[vmakarov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #11 from Vladimir Makarov  ---
I have patches fixing the two issues but when I started to test the patches I
found that LRA actually has >800 additional failures on power8 in comparison
with reload.  So I am going to look at this and try to fix this.

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[vmakarov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #10 from Vladimir Makarov  ---
(In reply to Alexandre Oliva from comment #6)
> Created attachment 37498 [details]
> Patch I'm testing to fix the bug
> 
> LRA wants harder than reload to avoid creating a stack slot to satisfy insn
> constraints.  As a result, it creates an additional REG:TI pseudo to reload
> a SUBREG:V2DF of a REG:TI, and then it tries to assign that pseudo to
> VSX_REGS, which in turn causes another reload because there's no way to load
> a TImode value into a VSX_REG in *mov_ppc64, and that requires
> another, and so on, until the limit on reload insns is exceeded.
> 
> The first problem is that we shouldn't be creating a TImode reload for
> VSX_REGS, since we can't possibly satisfy that: TImode values are not ok for
> VSX_REGS.  I've adjusted in_class_p to check HARD_REGNO_MODE_OK, and that
> put an end to infinite stream of reloads.
> 
> It was still a very long stream, though.  simplify_operand_subreg attempts
> to turn SUBREGs of MEMs into MEMs, but it will only proceed with the
> simplification if the resulting address is at least as valid as the
> original.  
> 
> Alas, instead of the simplification, we end up repeatedly generating reloads
> copying the initial value to stack slots with growing offsets, until the
> offset grows enough that the address becomes invalid, at which point the
> subreg simplification is performed.  That's 2047 excess stores and loads,
> plus insns that compute the stack address for each of them.
> 
> In order to fix that, I amended the test on whether to proceed with the
> subreg simplification to take into account the availability of regs that can
> hold a value of the intended mode in the goal class for that operand.
> 
> With that, we go from 2047 excess stores and loads to only 1.  I couldn't
> figure out yet how to get rid of this one extra store and load, and the
> excess stack slot, but I figured I'd share what I have, that I believe to be
> a solid fix, and save the investigation on an additional LRA improvement for
> later.

Alex, thanks for working on this.  The location of the fix is right.  But I
have a smaller fix, more general fix.  Instead of looking at the operand, i
propose to look that address will be valid when we use just base reg (LRA can
do this transformation later).  Original address is valid as TImode permits
such address (reg + offset).  For V2DFmode it is invalid as the mode permits
only reg[+reg] address, therefore we fail to remove the subreg.  Adding check
that just base address is valid, resolves the issue.

I'll test my patch on several machines and commit with your test if everything
is ok.
(In reply to Markus Trippelsdorf from comment #8)
> Hmm,
> 
> trippels@gcc2-power8 ~ % cat tramp3d-v4.ii
> class Init {
> public:
>   ~Init();
> } a;
> 
> trippels@gcc2-power8 ~ % g++ -flto -mlra tramp3d-v4.ii
> tramp3d-v4.ii: In function ‘__static_initialization_and_destruction_0’:
> tramp3d-v4.ii:4:4: internal compiler error: in lra_set_insn_recog_data, at
> lra.c:964
>  } a;
> ^
> 0x10553337 lra_set_insn_recog_data(rtx_insn*)
> ../../gcc/gcc/lra.c:962
> 0x10553b97 lra_get_insn_recog_data
> ../../gcc/gcc/lra-int.h:486
> 0x10553b97 lra_update_insn_regno_info
> ../../gcc/gcc/lra.c:1584
> 0x105540c7 lra_update_insn_regno_info
> ../../gcc/gcc/lra.c:1644
> 0x105540c7 lra_push_insn_1
> ../../gcc/gcc/lra.c:1649
> 0x105540c7 lra_push_insn
> ../../gcc/gcc/lra.c:1657
> 0x105540c7 push_insns
> ../../gcc/gcc/lra.c:1700
> 0x10556147 lra_process_new_insns(rtx_insn*, rtx_insn*, rtx_insn*, char
> const*)
> ../../gcc/gcc/lra.c:1746
> 0x1056dfcb curr_insn_transform
> ../../gcc/gcc/lra-constraints.c:3940
> 0x1057024b lra_constraints(bool)
> ../../gcc/gcc/lra-constraints.c:4427
> 0x1055482b lra(_IO_FILE*)
> ../../gcc/gcc/lra.c:2277
> 0x104f987b do_reload
> ../../gcc/gcc/ira.c:5393
> 0x104f987b execute
> ../../gcc/gcc/ira.c:5564

That is a different issue although it looks the same.  PPC port changed a lot
last year and nobody to pay attention to LRA because it is not a default local
RA.

The issue is in a new *toc_fusionload_di insn which uses unspec address and LRA
tries to reload it instead of just ignoring to do this.  I have a fix which
needs some testing.  The fix will be in the final patch.  I hope to commit it
on weekend or Monday.

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[vmakarov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #9 from Vladimir Makarov  ---
(In reply to Alexandre Oliva from comment #6)
> Created attachment 37498 [details]
> Patch I'm testing to fix the bug
> 
> LRA wants harder than reload to avoid creating a stack slot to satisfy insn
> constraints.  As a result, it creates an additional REG:TI pseudo to reload
> a SUBREG:V2DF of a REG:TI, and then it tries to assign that pseudo to
> VSX_REGS, which in turn causes another reload because there's no way to load
> a TImode value into a VSX_REG in *mov_ppc64, and that requires
> another, and so on, until the limit on reload insns is exceeded.
> 
> The first problem is that we shouldn't be creating a TImode reload for
> VSX_REGS, since we can't possibly satisfy that: TImode values are not ok for
> VSX_REGS.  I've adjusted in_class_p to check HARD_REGNO_MODE_OK, and that
> put an end to infinite stream of reloads.
> 
> It was still a very long stream, though.  simplify_operand_subreg attempts
> to turn SUBREGs of MEMs into MEMs, but it will only proceed with the
> simplification if the resulting address is at least as valid as the
> original.  
> 
> Alas, instead of the simplification, we end up repeatedly generating reloads
> copying the initial value to stack slots with growing offsets, until the
> offset grows enough that the address becomes invalid, at which point the
> subreg simplification is performed.  That's 2047 excess stores and loads,
> plus insns that compute the stack address for each of them.
> 
> In order to fix that, I amended the test on whether to proceed with the
> subreg simplification to take into account the availability of regs that can
> hold a value of the intended mode in the goal class for that operand.
> 
> With that, we go from 2047 excess stores and loads to only 1.  I couldn't
> figure out yet how to get rid of this one extra store and load, and the
> excess stack slot, but I figured I'd share what I have, that I believe to be
> a solid fix, and save the investigation on an additional LRA improvement for
> later.

Alex, thanks for working on this.  The location of the fix is right.  But I
have a smaller fix, more general fix.  Instead of looking at the operand, i
propose to look that address will be valid when we use just base reg (LRA can
do this transformation later).  Original address is valid as TImode permits
such address (reg + offset).  For V2DFmode it is invalid as the mode permits
only reg[+reg] address, therefore we fail to remove the subreg.  Adding check
that just base address is valid, resolves the issue.

I'll test my patch on several machines and commit with your test if everything
is ok.

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[trippels at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #7 from Markus Trippelsdorf  ---
Even with your patch applied, I still get the same ICE when I compile e.g.
tramp3d-v4.cpp with -flto on ppc64le:

trippels@gcc2-power8 ~ % g++ -w -Ofast -flto=16 -mlra tramp3d-v4.cpp
tramp3d-v4.cpp: In function
‘_ZN9DomainMapI8IntervalILi1EEiE10initializeERKS1_.isra.1374’:
tramp3d-v4.cpp:12683:0: internal compiler error: in lra_set_insn_recog_data, at
lra.c:964
...

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[trippels at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #8 from Markus Trippelsdorf  ---
Hmm,

trippels@gcc2-power8 ~ % cat tramp3d-v4.ii
class Init {
public:
  ~Init();
} a;

trippels@gcc2-power8 ~ % g++ -flto -mlra tramp3d-v4.ii
tramp3d-v4.ii: In function ‘__static_initialization_and_destruction_0’:
tramp3d-v4.ii:4:4: internal compiler error: in lra_set_insn_recog_data, at
lra.c:964
 } a;
^
0x10553337 lra_set_insn_recog_data(rtx_insn*)
../../gcc/gcc/lra.c:962
0x10553b97 lra_get_insn_recog_data
../../gcc/gcc/lra-int.h:486
0x10553b97 lra_update_insn_regno_info
../../gcc/gcc/lra.c:1584
0x105540c7 lra_update_insn_regno_info
../../gcc/gcc/lra.c:1644
0x105540c7 lra_push_insn_1
../../gcc/gcc/lra.c:1649
0x105540c7 lra_push_insn
../../gcc/gcc/lra.c:1657
0x105540c7 push_insns
../../gcc/gcc/lra.c:1700
0x10556147 lra_process_new_insns(rtx_insn*, rtx_insn*, rtx_insn*, char const*)
../../gcc/gcc/lra.c:1746
0x1056dfcb curr_insn_transform
../../gcc/gcc/lra-constraints.c:3940
0x1057024b lra_constraints(bool)
../../gcc/gcc/lra-constraints.c:4427
0x1055482b lra(_IO_FILE*)
../../gcc/gcc/lra.c:2277
0x104f987b do_reload
../../gcc/gcc/ira.c:5393
0x104f987b execute
../../gcc/gcc/ira.c:5564

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[aoliva at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

--- Comment #6 from Alexandre Oliva  ---
Created attachment 37498
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37498=edit
Patch I'm testing to fix the bug

LRA wants harder than reload to avoid creating a stack slot to satisfy insn
constraints.  As a result, it creates an additional REG:TI pseudo to reload a
SUBREG:V2DF of a REG:TI, and then it tries to assign that pseudo to VSX_REGS,
which in turn causes another reload because there's no way to load a TImode
value into a VSX_REG in *mov_ppc64, and that requires another, and so on,
until the limit on reload insns is exceeded.

The first problem is that we shouldn't be creating a TImode reload for
VSX_REGS, since we can't possibly satisfy that: TImode values are not ok for
VSX_REGS.  I've adjusted in_class_p to check HARD_REGNO_MODE_OK, and that put
an end to infinite stream of reloads.

It was still a very long stream, though.  simplify_operand_subreg attempts to
turn SUBREGs of MEMs into MEMs, but it will only proceed with the
simplification if the resulting address is at least as valid as the original.  

Alas, instead of the simplification, we end up repeatedly generating reloads
copying the initial value to stack slots with growing offsets, until the offset
grows enough that the address becomes invalid, at which point the subreg
simplification is performed.  That's 2047 excess stores and loads, plus insns
that compute the stack address for each of them.

In order to fix that, I amended the test on whether to proceed with the subreg
simplification to take into account the availability of regs that can hold a
value of the intended mode in the goal class for that operand.

With that, we go from 2047 excess stores and loads to only 1.  I couldn't
figure out yet how to get rid of this one extra store and load, and the excess
stack slot, but I figured I'd share what I have, that I believe to be a solid
fix, and save the investigation on an additional LRA improvement for later.

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[aoliva at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

Alexandre Oliva  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 CC||aoliva at gcc dot gnu.org
   Assignee|unassigned at gcc dot gnu.org  |aoliva at gcc dot 
gnu.org

--- Comment #5 from Alexandre Oliva  ---
I'm looking into this one

[Bug target/69461] [6 Regression] ICE in lra_set_insn_recog_data, at lra.c:964

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

Richard Biener  changed:

   What|Removed |Added

   Priority|P3  |P1
Version|5.3.0   |6.0
   Target Milestone|--- |6.0