[IRA] Fix PR91052 by skipping multiple_sets insn in combine_and_move_insns
diff mbox series

Message ID 38965b1a-6827-6edd-a708-26d6d1a02e77@linux.ibm.com
State New
Headers show
Series
  • [IRA] Fix PR91052 by skipping multiple_sets insn in combine_and_move_insns
Related show

Commit Message

Kewen.Lin Feb. 11, 2020, 8:01 a.m. UTC
Hi,

As PR91052's comments show, commit r272731 exposed one issue in function
combine_and_move_insns.  Function combine_and_move_insns perform the
below unexpected transformation.

** Before: **

   67: NOTE_INSN_BASIC_BLOCK 8
...
   59: {r184:SF=[sfp:SI-0x190];r121:SI=sfp:SI-0x190;}  ==> move object
      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: **

   67: NOTE_INSN_BASIC_BLOCK 8
...
   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;}   ==> moved here but update origin r121.
   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]                =====> with sp-0x18c; inconsistent from above.
      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

The insn 59 is special with multiple_sets, its movement alters the live
interval of r121 from insn 77 to insn 99 and update r121 with unexpected
value.

Bootstrapped/regtested on powerpc64le-linux-gnu (LE) and
ppc64-redhat-linux (BE).

Is it ok for trunk?

BR,
Kewen

-------

gcc/ChangeLog

2020-02-11  Kewen Lin  <linkw@gcc.gnu.org>

	* ira.c (combine_and_move_insns): Skip multiple_sets def_insn.

Comments

Vladimir Makarov Feb. 11, 2020, 4:24 p.m. UTC | #1
On 2/11/20 3:01 AM, Kewen.Lin wrote:
> Hi,
>
> As PR91052's comments show, commit r272731 exposed one issue in function
> combine_and_move_insns.  Function combine_and_move_insns perform the
> below unexpected transformation.
>
> ** Before: **
>
>     67: NOTE_INSN_BASIC_BLOCK 8
> ...
>     59: {r184:SF=[sfp:SI-0x190];r121:SI=sfp:SI-0x190;}  ==> move object
>        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: **
>
>     67: NOTE_INSN_BASIC_BLOCK 8
> ...
>     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;}   ==> moved here but update origin r121.
>     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]                =====> with sp-0x18c; inconsistent from above.
>        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
>
> The insn 59 is special with multiple_sets, its movement alters the live
> interval of r121 from insn 77 to insn 99 and update r121 with unexpected
> value.
>
> Bootstrapped/regtested on powerpc64le-linux-gnu (LE) and
> ppc64-redhat-linux (BE).
>
> Is it ok for trunk?

Yes. Thank you for working on the PR, Kewen.

I don't think that any expensive additional analysis is worth to use it 
for solving the PR.  So I believe your patch is an adequate solution.


> -------
>
> gcc/ChangeLog
>
> 2020-02-11  Kewen Lin  <linkw@gcc.gnu.org>
>
> 	* ira.c (combine_and_move_insns): Skip multiple_sets def_insn.
Kewen.Lin Feb. 12, 2020, 5:34 a.m. UTC | #2
on 2020/2/12 上午12:24, Vladimir Makarov wrote:
> On 2/11/20 3:01 AM, Kewen.Lin wrote:
>> Hi,
>>
>> As PR91052's comments show, commit r272731 exposed one issue in function
>> combine_and_move_insns.  Function combine_and_move_insns perform the
>> below unexpected transformation.
>>
>> ** Before: **
>>
>>     67: NOTE_INSN_BASIC_BLOCK 8
>> ...
>>     59: {r184:SF=[sfp:SI-0x190];r121:SI=sfp:SI-0x190;}  ==> move object
>>        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: **
>>
>>     67: NOTE_INSN_BASIC_BLOCK 8
>> ...
>>     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;}   ==> moved here but update origin r121.
>>     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]                =====> with sp-0x18c; inconsistent from above.
>>        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
>>
>> The insn 59 is special with multiple_sets, its movement alters the live
>> interval of r121 from insn 77 to insn 99 and update r121 with unexpected
>> value.
>>
>> Bootstrapped/regtested on powerpc64le-linux-gnu (LE) and
>> ppc64-redhat-linux (BE).
>>
>> Is it ok for trunk?
> 
> Yes. Thank you for working on the PR, Kewen.
> 
> I don't think that any expensive additional analysis is worth to use it for solving the PR.  So I believe your patch is an adequate solution.
> 

Thanks Vladimir!  Committed in r10-6591-g4d2248bec5d22061ab252724bd59d45c8a47e009
with the below updated ChangeLog (sorry for missing one PR line).

2020-02-12  Kewen Lin  <linkw@gcc.gnu.org>

	PR target/91052
	* ira.c (combine_and_move_insns): Skip multiple_sets def_insn.

And yes, I doubt the gain with more expensive analysis to legalize
the movement, even with that we need to update notes like REG_UNUSED
(inconsistent notes is direct cause of the ICE), it also seems not trivial.

BR,
Kewen

> 
>> -------
>>
>> gcc/ChangeLog
>>
>> 2020-02-11  Kewen Lin  <linkw@gcc.gnu.org>
>>
>>     * ira.c (combine_and_move_insns): Skip multiple_sets def_insn.
> 
>

Patch
diff mbox series

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