| Message ID | CACgzC7Dej5oZjSqaFLmYPdVSi0yP_f2v2obSaJmyEL1xvpPfGA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, May 23, 2014 at 9:23 AM, Zhenqiang Chen <zhenqiang.chen@linaro.org> wrote: > Hi, > > The patch fixes PR rtl-optimization/61278. Root cause for issue is > that df_live does not exist at -O1. > > Bootstrap and no make check regression on X86-64. > > OK for trunk? Why do you need to give up? It seems you can simply avoid marking the block as dirty (though df_get_live_in/out also hands you back DF_LR_IN/OUT if !df_live). So isn't the df_grow_bb_info the "real" fix? Note that df_get_live_in/out are functions tailored to IRA that knows that they handle both df_live and df_lr dependent on optimization level. Is shrink-wrapping supposed to work with both problems as well? Thanks, Richard. > Thanks! > -Zhenqiang > > ChangeLog: > 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> > > PR rtl-optimization/61278 > * shrink-wrap.c (move_insn_for_shrink_wrap): Check df_live. > > testsuite/ChangeLog: > 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> > > * gcc.dg/lto/pr61278_0.c: New test. > * gcc.dg/lto/pr61278_1.c: New test. > > diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c > index f09cfe7..be17829 100644 > --- a/gcc/shrink-wrap.c > +++ b/gcc/shrink-wrap.c > @@ -204,8 +204,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, > /* Create a new basic block on the edge. */ > if (EDGE_COUNT (next_block->preds) == 2) > { > + /* If DF_LIVE doesn't exist, i.e. at -O1, just give up. */ > + if (!df_live) > + return false; > + > next_block = split_edge (live_edge); > > + /* We create a new basic block. Call df_grow_bb_info to make sure > + all data structures are allocated. */ > + df_grow_bb_info (df_live); > bitmap_copy (df_get_live_in (next_block), df_get_live_out (bb)); > df_set_bb_dirty (next_block); > > diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_0.c > b/gcc/testsuite/gcc.dg/lto/pr61278_0.c > new file mode 100644 > index 0000000..03a24ae > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/pr61278_0.c > @@ -0,0 +1,30 @@ > +/* { dg-lto-do link } */ > +/* { dg-lto-options { { -flto -O0 } } } */ > +/* { dg-extra-ld-options " -flto -O1 " } */ > + > +static unsigned int > +fn1 (int p1, int p2) > +{ > + return 0; > +} > + > +char a, b, c; > + > +char > +foo (char *p) > +{ > + int i; > + for (b = 1 ; b > 0; b++) > + { > + for (i = 0; i < 2; i++) > + ; > + for (a = 1; a > 0; a++) > + { > + char d[1] = { 0 }; > + if (*p) > + break; > + c ^= fn1 (fn1 (fn1 (0, 0), 0), 0); > + } > + } > + return 0; > +} > diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_1.c > b/gcc/testsuite/gcc.dg/lto/pr61278_1.c > new file mode 100644 > index 0000000..b02c8ac > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/pr61278_1.c > @@ -0,0 +1,13 @@ > +/* { dg-lto-do link } */ > +/* { dg-lto-options { { -flto -O1 } } } */ > + > +extern char foo (char *); > + > +char d; > + > +int > +main () > +{ > + foo (&d); > + return 0; > +}
On 23 May 2014 17:05, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, May 23, 2014 at 9:23 AM, Zhenqiang Chen > <zhenqiang.chen@linaro.org> wrote: >> Hi, >> >> The patch fixes PR rtl-optimization/61278. Root cause for issue is >> that df_live does not exist at -O1. >> >> Bootstrap and no make check regression on X86-64. >> >> OK for trunk? > > Why do you need to give up? It seems you can simply avoid > marking the block as dirty (though df_get_live_in/out also hands you > back DF_LR_IN/OUT if !df_live). So isn't the df_grow_bb_info the > "real" fix? The df_get_live_in of the new basic block will be used to analyse later INSNs. If it is not set or incorrect, it will impact on later analysis. df_grow_bb_info is to make sure the live_in data structure is allocated for the new basic block (although I have not found any case fail without it). After bitmap_copy(...), we can use it for later INSNs. > Note that df_get_live_in/out are functions tailored to IRA that > knows that they handle both df_live and df_lr dependent on > optimization level. Is shrink-wrapping supposed to work with > both problems as well? Yes. But it seams not perfect to handle df_lr problem. When I fixed PR 57637 (https://gcc.gnu.org/ml/gcc-patches/2013-07/msg00897.html), we selected "if DF_LIVE doesn't exist, i.e. at -O1, just give up searching NEXT_BLOCK." Thanks! -Zhenqiang >> >> ChangeLog: >> 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >> >> PR rtl-optimization/61278 >> * shrink-wrap.c (move_insn_for_shrink_wrap): Check df_live. >> >> testsuite/ChangeLog: >> 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >> >> * gcc.dg/lto/pr61278_0.c: New test. >> * gcc.dg/lto/pr61278_1.c: New test. >> >> diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c >> index f09cfe7..be17829 100644 >> --- a/gcc/shrink-wrap.c >> +++ b/gcc/shrink-wrap.c >> @@ -204,8 +204,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, >> /* Create a new basic block on the edge. */ >> if (EDGE_COUNT (next_block->preds) == 2) >> { >> + /* If DF_LIVE doesn't exist, i.e. at -O1, just give up. */ >> + if (!df_live) >> + return false; >> + >> next_block = split_edge (live_edge); >> >> + /* We create a new basic block. Call df_grow_bb_info to make sure >> + all data structures are allocated. */ >> + df_grow_bb_info (df_live); >> bitmap_copy (df_get_live_in (next_block), df_get_live_out (bb)); >> df_set_bb_dirty (next_block); >> >> diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_0.c >> b/gcc/testsuite/gcc.dg/lto/pr61278_0.c >> new file mode 100644 >> index 0000000..03a24ae >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/lto/pr61278_0.c >> @@ -0,0 +1,30 @@ >> +/* { dg-lto-do link } */ >> +/* { dg-lto-options { { -flto -O0 } } } */ >> +/* { dg-extra-ld-options " -flto -O1 " } */ >> + >> +static unsigned int >> +fn1 (int p1, int p2) >> +{ >> + return 0; >> +} >> + >> +char a, b, c; >> + >> +char >> +foo (char *p) >> +{ >> + int i; >> + for (b = 1 ; b > 0; b++) >> + { >> + for (i = 0; i < 2; i++) >> + ; >> + for (a = 1; a > 0; a++) >> + { >> + char d[1] = { 0 }; >> + if (*p) >> + break; >> + c ^= fn1 (fn1 (fn1 (0, 0), 0), 0); >> + } >> + } >> + return 0; >> +} >> diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_1.c >> b/gcc/testsuite/gcc.dg/lto/pr61278_1.c >> new file mode 100644 >> index 0000000..b02c8ac >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/lto/pr61278_1.c >> @@ -0,0 +1,13 @@ >> +/* { dg-lto-do link } */ >> +/* { dg-lto-options { { -flto -O1 } } } */ >> + >> +extern char foo (char *); >> + >> +char d; >> + >> +int >> +main () >> +{ >> + foo (&d); >> + return 0; >> +}
On Fri, May 23, 2014 at 12:33 PM, Zhenqiang Chen <zhenqiang.chen@linaro.org> wrote: > On 23 May 2014 17:05, Richard Biener <richard.guenther@gmail.com> wrote: >> On Fri, May 23, 2014 at 9:23 AM, Zhenqiang Chen >> <zhenqiang.chen@linaro.org> wrote: >>> Hi, >>> >>> The patch fixes PR rtl-optimization/61278. Root cause for issue is >>> that df_live does not exist at -O1. >>> >>> Bootstrap and no make check regression on X86-64. >>> >>> OK for trunk? >> >> Why do you need to give up? It seems you can simply avoid >> marking the block as dirty (though df_get_live_in/out also hands you >> back DF_LR_IN/OUT if !df_live). So isn't the df_grow_bb_info the >> "real" fix? > > The df_get_live_in of the new basic block will be used to analyse > later INSNs. If it is not set or incorrect, it will impact on later > analysis. > df_grow_bb_info is to make sure the live_in data structure is > allocated for the new basic block (although I have not found any case > fail without it). After bitmap_copy(...), we can use it for later > INSNs. > >> Note that df_get_live_in/out are functions tailored to IRA that >> knows that they handle both df_live and df_lr dependent on >> optimization level. Is shrink-wrapping supposed to work with >> both problems as well? > > Yes. But it seams not perfect to handle df_lr problem. When I fixed PR > 57637 (https://gcc.gnu.org/ml/gcc-patches/2013-07/msg00897.html), we > selected "if DF_LIVE doesn't exist, i.e. at -O1, just give up > searching NEXT_BLOCK." Ok, I see. Maybe it would be better to completely disable shrink-wrapping when LIVE is not available. Patch is ok. Thanks, Richard. > Thanks! > -Zhenqiang > >>> >>> ChangeLog: >>> 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >>> >>> PR rtl-optimization/61278 >>> * shrink-wrap.c (move_insn_for_shrink_wrap): Check df_live. >>> >>> testsuite/ChangeLog: >>> 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >>> >>> * gcc.dg/lto/pr61278_0.c: New test. >>> * gcc.dg/lto/pr61278_1.c: New test. >>> >>> diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c >>> index f09cfe7..be17829 100644 >>> --- a/gcc/shrink-wrap.c >>> +++ b/gcc/shrink-wrap.c >>> @@ -204,8 +204,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, >>> /* Create a new basic block on the edge. */ >>> if (EDGE_COUNT (next_block->preds) == 2) >>> { >>> + /* If DF_LIVE doesn't exist, i.e. at -O1, just give up. */ >>> + if (!df_live) >>> + return false; >>> + >>> next_block = split_edge (live_edge); >>> >>> + /* We create a new basic block. Call df_grow_bb_info to make sure >>> + all data structures are allocated. */ >>> + df_grow_bb_info (df_live); >>> bitmap_copy (df_get_live_in (next_block), df_get_live_out (bb)); >>> df_set_bb_dirty (next_block); >>> >>> diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_0.c >>> b/gcc/testsuite/gcc.dg/lto/pr61278_0.c >>> new file mode 100644 >>> index 0000000..03a24ae >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/pr61278_0.c >>> @@ -0,0 +1,30 @@ >>> +/* { dg-lto-do link } */ >>> +/* { dg-lto-options { { -flto -O0 } } } */ >>> +/* { dg-extra-ld-options " -flto -O1 " } */ >>> + >>> +static unsigned int >>> +fn1 (int p1, int p2) >>> +{ >>> + return 0; >>> +} >>> + >>> +char a, b, c; >>> + >>> +char >>> +foo (char *p) >>> +{ >>> + int i; >>> + for (b = 1 ; b > 0; b++) >>> + { >>> + for (i = 0; i < 2; i++) >>> + ; >>> + for (a = 1; a > 0; a++) >>> + { >>> + char d[1] = { 0 }; >>> + if (*p) >>> + break; >>> + c ^= fn1 (fn1 (fn1 (0, 0), 0), 0); >>> + } >>> + } >>> + return 0; >>> +} >>> diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_1.c >>> b/gcc/testsuite/gcc.dg/lto/pr61278_1.c >>> new file mode 100644 >>> index 0000000..b02c8ac >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/pr61278_1.c >>> @@ -0,0 +1,13 @@ >>> +/* { dg-lto-do link } */ >>> +/* { dg-lto-options { { -flto -O1 } } } */ >>> + >>> +extern char foo (char *); >>> + >>> +char d; >>> + >>> +int >>> +main () >>> +{ >>> + foo (&d); >>> + return 0; >>> +}
On 23 May 2014 19:56, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, May 23, 2014 at 12:33 PM, Zhenqiang Chen > <zhenqiang.chen@linaro.org> wrote: >> On 23 May 2014 17:05, Richard Biener <richard.guenther@gmail.com> wrote: >>> On Fri, May 23, 2014 at 9:23 AM, Zhenqiang Chen >>> <zhenqiang.chen@linaro.org> wrote: >>>> Hi, >>>> >>>> The patch fixes PR rtl-optimization/61278. Root cause for issue is >>>> that df_live does not exist at -O1. >>>> >>>> Bootstrap and no make check regression on X86-64. >>>> >>>> OK for trunk? >>> >>> Why do you need to give up? It seems you can simply avoid >>> marking the block as dirty (though df_get_live_in/out also hands you >>> back DF_LR_IN/OUT if !df_live). So isn't the df_grow_bb_info the >>> "real" fix? >> >> The df_get_live_in of the new basic block will be used to analyse >> later INSNs. If it is not set or incorrect, it will impact on later >> analysis. >> df_grow_bb_info is to make sure the live_in data structure is >> allocated for the new basic block (although I have not found any case >> fail without it). After bitmap_copy(...), we can use it for later >> INSNs. >> >>> Note that df_get_live_in/out are functions tailored to IRA that >>> knows that they handle both df_live and df_lr dependent on >>> optimization level. Is shrink-wrapping supposed to work with >>> both problems as well? >> >> Yes. But it seams not perfect to handle df_lr problem. When I fixed PR >> 57637 (https://gcc.gnu.org/ml/gcc-patches/2013-07/msg00897.html), we >> selected "if DF_LIVE doesn't exist, i.e. at -O1, just give up >> searching NEXT_BLOCK." > > Ok, I see. Maybe it would be better to completely disable > shrink-wrapping when LIVE is not available. Agree. There are potential bugs since DF_REF_PARTIAL and DF_REF_CONDITIONAL defs are not correctly handled in DF_LR. I will send out a patch to disable prepare_shrink_wrap when DF_LIVE is not available. > Patch is ok. Thanks! Committed @210922. -Zhenqiang > Thanks, > Richard. > >> Thanks! >> -Zhenqiang >> >>>> >>>> ChangeLog: >>>> 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >>>> >>>> PR rtl-optimization/61278 >>>> * shrink-wrap.c (move_insn_for_shrink_wrap): Check df_live. >>>> >>>> testsuite/ChangeLog: >>>> 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >>>> >>>> * gcc.dg/lto/pr61278_0.c: New test. >>>> * gcc.dg/lto/pr61278_1.c: New test. >>>> >>>> diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c >>>> index f09cfe7..be17829 100644 >>>> --- a/gcc/shrink-wrap.c >>>> +++ b/gcc/shrink-wrap.c >>>> @@ -204,8 +204,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, >>>> /* Create a new basic block on the edge. */ >>>> if (EDGE_COUNT (next_block->preds) == 2) >>>> { >>>> + /* If DF_LIVE doesn't exist, i.e. at -O1, just give up. */ >>>> + if (!df_live) >>>> + return false; >>>> + >>>> next_block = split_edge (live_edge); >>>> >>>> + /* We create a new basic block. Call df_grow_bb_info to make sure >>>> + all data structures are allocated. */ >>>> + df_grow_bb_info (df_live); >>>> bitmap_copy (df_get_live_in (next_block), df_get_live_out (bb)); >>>> df_set_bb_dirty (next_block); >>>> >>>> diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_0.c >>>> b/gcc/testsuite/gcc.dg/lto/pr61278_0.c >>>> new file mode 100644 >>>> index 0000000..03a24ae >>>> --- /dev/null >>>> +++ b/gcc/testsuite/gcc.dg/lto/pr61278_0.c >>>> @@ -0,0 +1,30 @@ >>>> +/* { dg-lto-do link } */ >>>> +/* { dg-lto-options { { -flto -O0 } } } */ >>>> +/* { dg-extra-ld-options " -flto -O1 " } */ >>>> + >>>> +static unsigned int >>>> +fn1 (int p1, int p2) >>>> +{ >>>> + return 0; >>>> +} >>>> + >>>> +char a, b, c; >>>> + >>>> +char >>>> +foo (char *p) >>>> +{ >>>> + int i; >>>> + for (b = 1 ; b > 0; b++) >>>> + { >>>> + for (i = 0; i < 2; i++) >>>> + ; >>>> + for (a = 1; a > 0; a++) >>>> + { >>>> + char d[1] = { 0 }; >>>> + if (*p) >>>> + break; >>>> + c ^= fn1 (fn1 (fn1 (0, 0), 0), 0); >>>> + } >>>> + } >>>> + return 0; >>>> +} >>>> diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_1.c >>>> b/gcc/testsuite/gcc.dg/lto/pr61278_1.c >>>> new file mode 100644 >>>> index 0000000..b02c8ac >>>> --- /dev/null >>>> +++ b/gcc/testsuite/gcc.dg/lto/pr61278_1.c >>>> @@ -0,0 +1,13 @@ >>>> +/* { dg-lto-do link } */ >>>> +/* { dg-lto-options { { -flto -O1 } } } */ >>>> + >>>> +extern char foo (char *); >>>> + >>>> +char d; >>>> + >>>> +int >>>> +main () >>>> +{ >>>> + foo (&d); >>>> + return 0; >>>> +}
diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c index f09cfe7..be17829 100644 --- a/gcc/shrink-wrap.c +++ b/gcc/shrink-wrap.c @@ -204,8 +204,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, /* Create a new basic block on the edge. */ if (EDGE_COUNT (next_block->preds) == 2) { + /* If DF_LIVE doesn't exist, i.e. at -O1, just give up. */ + if (!df_live) + return false; + next_block = split_edge (live_edge); + /* We create a new basic block. Call df_grow_bb_info to make sure + all data structures are allocated. */ + df_grow_bb_info (df_live); bitmap_copy (df_get_live_in (next_block), df_get_live_out (bb)); df_set_bb_dirty (next_block); diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_0.c b/gcc/testsuite/gcc.dg/lto/pr61278_0.c new file mode 100644 index 0000000..03a24ae --- /dev/null +++ b/gcc/testsuite/gcc.dg/lto/pr61278_0.c @@ -0,0 +1,30 @@ +/* { dg-lto-do link } */ +/* { dg-lto-options { { -flto -O0 } } } */ +/* { dg-extra-ld-options " -flto -O1 " } */ + +static unsigned int +fn1 (int p1, int p2) +{ + return 0; +} + +char a, b, c; + +char +foo (char *p) +{ + int i; + for (b = 1 ; b > 0; b++) + { + for (i = 0; i < 2; i++) + ; + for (a = 1; a > 0; a++) + { + char d[1] = { 0 }; + if (*p) + break; + c ^= fn1 (fn1 (fn1 (0, 0), 0), 0); + } + } + return 0; +} diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_1.c b/gcc/testsuite/gcc.dg/lto/pr61278_1.c new file mode 100644 index 0000000..b02c8ac --- /dev/null +++ b/gcc/testsuite/gcc.dg/lto/pr61278_1.c @@ -0,0 +1,13 @@ +/* { dg-lto-do link } */ +/* { dg-lto-options { { -flto -O1 } } } */ + +extern char foo (char *); + +char d; + +int +main () +{ + foo (&d); + return 0; +}
Hi, The patch fixes PR rtl-optimization/61278. Root cause for issue is that df_live does not exist at -O1. Bootstrap and no make check regression on X86-64. OK for trunk? Thanks! -Zhenqiang ChangeLog: 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> PR rtl-optimization/61278 * shrink-wrap.c (move_insn_for_shrink_wrap): Check df_live. testsuite/ChangeLog: 2014-05-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> * gcc.dg/lto/pr61278_0.c: New test. * gcc.dg/lto/pr61278_1.c: New test.