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