[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[cvs-commit at gcc dot gnu.org]

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

--- Comment #16 from CVS Commits  ---
The master branch has been updated by Kewen Lin :

https://gcc.gnu.org/g:4d2248bec5d22061ab252724bd59d45c8a47e009

commit r10-6591-g4d2248bec5d22061ab252724bd59d45c8a47e009
Author: Kewen Lin 
Date:   Tue Feb 11 23:22:02 2020 -0600

[IRA] Fix PR91052 by skipping multiple_sets insn in combine_and_move_insns

As PR91052's comments show, commit r272731 exposed one issue in function
combine_and_move_insns.  Function combine_and_move_insns perform the
unexpected movement which alter live interval of some register, leading
incorrect value to be used.  See PR91052 for details.

2020-02-12  Kewen Lin  
PR target/91052
* ira.c (combine_and_move_insns): Skip multiple_sets def_insn.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[linkw at gcc dot gnu.org]

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

Kewen Lin  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #17 from Kewen Lin  ---
Should be fixed with r10-6591.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

   Keywords|needs-bisection |
 Status|UNCONFIRMED |NEW
   Last reconfirmed||2019-08-08
 CC||linkw at gcc dot gnu.org,
   ||marxin at gcc dot gnu.org
  Known to work||9.1.0
 Ever confirmed|0   |1
  Known to fail||10.0

--- Comment #2 from Martin Liška  ---
Started with r272731.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[rguenth at gcc dot gnu.org]

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

Richard Biener  changed:

   What|Removed |Added

   Keywords||needs-bisection
 CC||rsandifo at gcc dot gnu.org
   Target Milestone|--- |10.0

--- Comment #1 from Richard Biener  ---
Not sure if recent, just CCing suspect ;)  Needs bisection anyways.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[asolokha at gmx dot com]

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

--- Comment #3 from Arseny Solokha  ---
I cannot reproduce it anymore as of r279405.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[marxin at gcc dot gnu.org]

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

--- Comment #4 from Martin Liška  ---
I can still reproduce it with r279828.

$ /dev/shm/gcc-objdir-bisect/gcc/xg++ -B/dev/shm/gcc-objdir-bisect/gcc
/home/marxin/Programming/gcc/gcc/testsuite/gfortran.dg/vect/cost-model-pr34445.f
-misel -O2 -fstack-protector -funroll-all-loops -fno-sched-pressure
-fno-tree-ch -fno-tree-forwprop -fno-tree-ter -c
during RTL pass: ira
/home/marxin/Programming/gcc/gcc/testsuite/gfortran.dg/vect/cost-model-pr34445.f:8:0:

8 |   End
  | 
internal compiler error: in fix_reg_equiv_init, at ira.c:2682
0x7f619b2b8e0a __libc_start_main
../csu/libc-start.c:308
0x863119 ???
../sysdeps/x86_64/start.S:120
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[asolokha at gmx dot com]

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

Arseny Solokha  changed:

   What|Removed |Added

 Target|powerpc-*-linux-gnu |

--- Comment #5 from Arseny Solokha  ---
Thanks. It's not specific to powerpc, then.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[marxin at gcc dot gnu.org]

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

--- Comment #6 from Martin Liška  ---
(In reply to Arseny Solokha from comment #5)
> Thanks. It's not specific to powerpc, then.

Well, it is probably, I'm testing powerpc-e300c3 cross compiler.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[rguenth at gcc dot gnu.org]

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

Richard Biener  changed:

   What|Removed |Added

   Priority|P3  |P1

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[marxin at gcc dot gnu.org]

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

--- Comment #7 from Martin Liška  ---
@Kewen: Can you please take a look?

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[linkw at gcc dot gnu.org]

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

--- Comment #8 from Kewen Lin  ---
Sorry, I just saw this bug was starting to fail with my commit. Thanks for @ing
me! My commit is just to pass the finiteness information down to RTL phase. The
loops in that case are simple and have only an exit respectively, it should be
taken as finite loop, I think my commit just exposed some bugs.

I'll take vacation for two weeks, will dig into this then.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[linkw at gcc dot gnu.org]

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

--- Comment #9 from Kewen Lin  ---
I can't reproduce this on both powerpc64le-linux-gnu
(edabbec31e3bfc9a9757f80c8610706ed00e5a1a) and ppc64-redhat-linux (r278916),
IIUC I need the powerpc-e300c3 environment header/library as sysroot for
cross-compiler build. Could you guide me how to get it? Does it exist somewhere
in some machine in Compiler Farm?

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[marxin at gcc dot gnu.org]

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

--- Comment #10 from Martin Liška  ---
(In reply to Kewen Lin from comment #9)
> I can't reproduce this on both powerpc64le-linux-gnu
> (edabbec31e3bfc9a9757f80c8610706ed00e5a1a) and ppc64-redhat-linux (r278916),
> IIUC I need the powerpc-e300c3 environment header/library as sysroot for
> cross-compiler build. Could you guide me how to get it? Does it exist
> somewhere in some machine in Compiler Farm?

You should not need it. One only needs to build make-host (front-ends).

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[linkw at gcc dot gnu.org]

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

--- Comment #11 from Kewen Lin  ---
Thanks a lot for Martin's help on reproduction. I can reproduce this ICE and
confirmed that if commenting out my patch r272731 in loop-iv.c, it can pass. I
can also reproduce it on powerpc64 (ppc64-redhat-linux) with -m32.

Looking into the source code of the case, the bound of the loop NSphInp is an
automatic variable and uninitialized, but we still can assert the loop is
finite since it will wrap.

* The ICE occurs on the RTL insn: *

(insn 219 60 61 8 (parallel [
  (set (reg:SF 184 [ _2 ])
  (mem:SF (plus:SI (reg/f:SI 110 sfp)
  (const_int -400 [0xfe70])) [1
MEM[base: _22, offset: 0B]+0 S4 A32]))
  (set (reg:SI 121 [ ivtmp.11 ])
  (plus:SI (reg/f:SI 110 sfp)
  (const_int -400 [0xfe70])))
  ])
"/home/linkw/gcc/gcc-git/gcc/testsuite/gfortran.dg/vect/cost-model-pr34445.f":5:0
659 {*movsf_update1}
   (nil))

The assertion requires it should have single set but this one doesn't.

* In 280r.sched1: *
   67: NOTE_INSN_BASIC_BLOCK 8
  ...
  103: NOTE_INSN_DELETED
   59: {r184:SF=[sfp:SI-0x190];r121:SI=sfp:SI-0x190;}
  REG_UNUSED r121:SI
   77: {r191:SF=[sfp:SI-0x18c];r121:SI=sfp:SI-0x18c;}
   60: r122:SI=r127:SI
  REG_DEAD r127:SI
   61: [r122:SI]=r184:SF
  REG_DEAD r184:SF
   79: [++r122:SI]=r191:SF
  REG_DEAD r191:SF
  REG_INC r122:SI
   ...

* Entering ira 282r.ira: *

  59 gets deleted and generates 219 as below.

   67: NOTE_INSN_BASIC_BLOCK 8
  ...
  103: NOTE_INSN_DELETED
   77: {r191:SF=[sfp:SI-0x18c];r121:SI=sfp:SI-0x18c;}
  REG_UNUSED r121:SI
   60: r122:SI=r127:SI
  REG_DEAD r127:SI
  219: {r184:SF=[sfp:SI-0x190];r121:SI=sfp:SI-0x190;}
   61: [r122:SI]=r184:SF
  REG_DEAD r184:SF
   79: [++r122:SI]=r191:SF
  REG_DEAD r191:SF
  REG_INC r122:SI

1) It can pass with -mno-update since we don't generate that update-form insn
parallel with two SETs.
2) If we disable ira_conflicts_p, it can also pass. I can see the update-form
assembly is generated well. So I think my patch just exposes some ira issue.
3) The original insn 59 has REG_UNUSED, but the new one 219 was put after 77,
then 77 has REG_UNUSED but 219 doesn't, I'll look into why. I assume the
assertion "ira_assert (set != NULL_RTX)" would match some previous analysis
(criteria into the queue), there may be something unexpected.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[linkw at gcc dot gnu.org]

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

Kewen Lin  changed:

   What|Removed |Added

 CC||amodra at gcc dot gnu.org

--- Comment #12 from Kewen Lin  ---
The insn 59 is deleted and new insn 219 is generated by function
combine_and_move_insns, the failure is due to that:
  Originally insn 59 has the note that the updated reg r121 is REG_UNUSED.
  Later insn 219 inserted before r184's use, with note directly copied from
insn 59, r121 is still REG_UNUSED.
  So function setup_reg_equiv doesn't filter it out since it's taken as
single_set due to REG_UNUSED existence.

But the r121 in insn 219 isn't actually REG_UNUSED and gets fixed after live
info recalculation as below:

   67: NOTE_INSN_BASIC_BLOCK 8
  ...
  103: NOTE_INSN_DELETED
   77: {r191:SF=[sfp:SI-0x18c];r121:SI=sfp:SI-0x18c;}
  REG_UNUSED r121:SI
   60: r122:SI=r127:SI
  REG_DEAD r127:SI
  219: {r184:SF=[sfp:SI-0x190];r121:SI=sfp:SI-0x190;}
   61: [r122:SI]=r184:SF
  REG_DEAD r184:SF
   79: [++r122:SI]=r191:SF
  REG_DEAD r191:SF
  REG_INC r122:SI

Then fix_reg_equiv_init find the insn isn't single_set and assertion fail.

The newly generated doesn't look incorrect since some semantic changes as
below.

* Before combine_and_move_insns, we have:
   67: NOTE_INSN_BASIC_BLOCK 8
   58: NOTE_INSN_DELETED
   63: NOTE_INSN_DELETED
  110: NOTE_INSN_DELETED
   88: NOTE_INSN_DELETED
   76: NOTE_INSN_DELETED
  103: NOTE_INSN_DELETED
   59: {r184:SF=[sfp:SI-0x190];r121:SI=sfp:SI-0x190;}
  REG_UNUSED r121:SI
   77: {r191:SF=[sfp:SI-0x18c];r121:SI=sfp:SI-0x18c;}
   60: r122:SI=r127:SI
  REG_DEAD r127:SI
   61: [r122:SI]=r184:SF
  REG_DEAD r184:SF
   79: [++r122:SI]=r191:SF
  REG_DEAD r191:SF
  REG_INC r122:SI
   64: r187:SF=[r137:SI+low(`*.LC0')]
   99: r198:SF=[++r121:SI] => with sp-0x18c+4;
  REG_INC r121:SI
  104: r201:SF=[r137:SI+low(`*.LC0')]
   65: [r126:SI]=r187:SF
  REG_DEAD r187:SF
  105: [r126:SI]=r201:SF
  REG_DEAD r201:SF
  101: [++r122:SI]=r198:SF
  REG_DEAD r198:SF
  REG_INC r122:SI
  114: L114:
  113: NOTE_INSN_BASIC_BLOCK 9

* After combine_and_move_insns, we have:

   67: NOTE_INSN_BASIC_BLOCK 8
   58: NOTE_INSN_DELETED
   63: NOTE_INSN_DELETED
  110: NOTE_INSN_DELETED
   88: NOTE_INSN_DELETED
   76: NOTE_INSN_DELETED
  103: NOTE_INSN_DELETED
   77: {r191:SF=[sfp:SI-0x18c];r121:SI=sfp:SI-0x18c;}
  REG_UNUSED r121:SI
   60: r122:SI=r127:SI
  REG_DEAD r127:SI
  219: {r184:SF=[sfp:SI-0x190];r121:SI=sfp:SI-0x190;}
   61: [r122:SI]=r184:SF
  REG_DEAD r184:SF
   79: [++r122:SI]=r191:SF
  REG_DEAD r191:SF
  REG_INC r122:SI
   64: r187:SF=[r137:SI+low(`*.LC0')]
  REG_EQUIV [r137:SI+low(`*.LC0')]
   99: r198:SF=[++r121:SI]=> still with sp-0x18c;
  REG_INC r121:SI
  104: r201:SF=[r137:SI+low(`*.LC0')]
  REG_EQUIV [r137:SI+low(`*.LC0')]
   65: [r126:SI]=r187:SF
  REG_DEAD r187:SF
  105: [r126:SI]=r201:SF
  REG_DEAD r201:SF
  101: [++r122:SI]=r198:SF
  REG_DEAD r198:SF
  REG_INC r122:SI
  114: L114:
  113: NOTE_INSN_BASIC_BLOCK 9

As above, I guess we can guard the strict single_set condition for function
combine_and_move_insns.

@@ -3827,7 +3874,7 @@ combine_and_move_insns (void)

   /* Move the initialization of the register to just before
 USE_INSN.  Update the flow information.  */
-  else if (prev_nondebug_insn (use_insn) != def_insn)
+  else if (prev_nondebug_insn (use_insn) != def_insn && !multiple_sets
(def_insn))
{
  rtx_insn *new_insn;

Or any alternative to ensure multiple sets movement doesn't kill usused reg's
live range and notes gets updated.

-

Hi @Alan/@Vladimir,

What do you think of the issue? Thanks!

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[linkw at gcc dot gnu.org]

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

--- Comment #13 from Kewen Lin  ---
> “The newly generated doesn't look incorrect since some semantic changes as 
> below.”

Sorry, typo, it should be "The newly generated insn doesn't look correct since
some semantic changes as below."

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[amodra at gmail dot com]

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

Alan Modra  changed:

   What|Removed |Added

 CC||amodra at gmail dot com

--- Comment #14 from Alan Modra  ---
I agree with your analysis, and that combine_and_move_insns has a bug.  (That
doesn't mean I should be viewed as any sort of expert on ira.c.  The fact that
my name appears on git blame for much of combine_and_move_insns is just due to
me splitting out existing code into that function!)

"!multiple_sets (def_insn)" seems the correct solution too, but I'd be inclined
to move that test earlier, either before or after can_throw_internal on the
grounds that asm insns might have the same problem with multiple sets.  Perhaps
comment that instructions with multiple sets can only be moved if DF analysis
is performed for all of the registers set.

[Bug target/91052] [10 Regression] ICE in fix_reg_equiv_init, at ira.c:2705

[linkw at gcc dot gnu.org]

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

Kewen Lin  changed:

   What|Removed |Added

 CC||bergner at gcc dot gnu.org,
   ||segher at gcc dot gnu.org,
   ||wschmidt at gcc dot gnu.org
   Assignee|unassigned at gcc dot gnu.org  |linkw at gcc dot gnu.org

--- Comment #15 from Kewen Lin  ---
Thanks for your comments Alan! I've updated the patch listed below as your
suggestion:

diff --git a/gcc/ira.c b/gcc/ira.c
index c8b5f86..a655ae1 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -3784,6 +3784,11 @@ combine_and_move_insns (void)
   if (can_throw_internal (def_insn))
continue;

+  /* Instructions with multiple sets can only be moved if DF analysis is
+performed for all of the registers set.  See PR91052.  */
+  if (multiple_sets (def_insn))
+   continue;
+
   basic_block use_bb = BLOCK_FOR_INSN (use_insn);
   basic_block def_bb = BLOCK_FOR_INSN (def_insn);
   if (bb_loop_depth (use_bb) > bb_loop_depth (def_bb))

Bootstrapped/regtested on powerpc64le-linux-gnu (LE), the testing on
ppc64-redhat-linux (BE) is still ongoing.

I'll send it to gcc-patches@ for review next week if no more comments received.