| Message ID | e825d821-73a8-5e5a-4557-51429854def0@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ipa-fnsummary: Remove inconsistent bp_pack_value | expand |
On Fri, Sep 17, 2021 at 11:43 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > > Hi, > > When changing target_info with bitfield, I happened to find this > inconsistent streaming in and out. We have the streaming in: > > bp_pack_value (&bp, info->inlinable, 1); > bp_pack_value (&bp, false, 1); > bp_pack_value (&bp, info->fp_expressions, 1); > > while the streaming out: > > info->inlinable = bp_unpack_value (&bp, 1); > info->fp_expressions = bp_unpack_value (&bp, 1) > > The cleanup of Cilk Plus support seemed to miss to remove the bit > streaming out but change with streaming false. > > By hacking fp_expression_p to return true always, I can see it > reads the wrong fp_expressions value (false) out in wpa dumping. > > Bootstrapped and regress-tested on powerpc64le-linux-gnu Power9. > > Is it ok for trunk? OK for trunk and all affected branches (note we need to bump the LTO minor version there). The issue comes from the removal of cilk+ in r8-4956 which removed the bp_unpack but replaced the bp_pack ... It's a correctness issue as we'll read fp_expressions as always 'false' Thanks, Richard. > > BR, > Kewen > ----- > gcc/ChangeLog: > > * ipa-fnsummary.c (ipa_fn_summary_write): Remove inconsistent > bitfield streaming out. > > diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c > index 2470937460f..31199919405 100644 > --- a/gcc/ipa-fnsummary.c > +++ b/gcc/ipa-fnsummary.c > @@ -4652,7 +4652,6 @@ ipa_fn_summary_write (void) > info->time.stream_out (ob); > bp = bitpack_create (ob->main_stream); > bp_pack_value (&bp, info->inlinable, 1); > - bp_pack_value (&bp, false, 1); > bp_pack_value (&bp, info->fp_expressions, 1); > streamer_write_bitpack (&bp); > streamer_write_uhwi (ob, vec_safe_length (info->conds));
On Fri, Sep 17, 2021 at 12:03 PM Richard Biener <richard.guenther@gmail.com> wrote: > > On Fri, Sep 17, 2021 at 11:43 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > > > > Hi, > > > > When changing target_info with bitfield, I happened to find this > > inconsistent streaming in and out. We have the streaming in: > > > > bp_pack_value (&bp, info->inlinable, 1); > > bp_pack_value (&bp, false, 1); > > bp_pack_value (&bp, info->fp_expressions, 1); > > > > while the streaming out: > > > > info->inlinable = bp_unpack_value (&bp, 1); > > info->fp_expressions = bp_unpack_value (&bp, 1) > > > > The cleanup of Cilk Plus support seemed to miss to remove the bit > > streaming out but change with streaming false. > > > > By hacking fp_expression_p to return true always, I can see it > > reads the wrong fp_expressions value (false) out in wpa dumping. > > > > Bootstrapped and regress-tested on powerpc64le-linux-gnu Power9. > > > > Is it ok for trunk? > > OK for trunk and all affected branches (note we need to bump the > LTO minor version there). The issue comes from the removal > of cilk+ in r8-4956 which removed the bp_unpack but replaced > the bp_pack ... > > It's a correctness issue as we'll read fp_expressions as always 'false' Btw, on branches we could also simply unpack a dummy bit to avoid changing the format. > Thanks, > Richard. > > > > > BR, > > Kewen > > ----- > > gcc/ChangeLog: > > > > * ipa-fnsummary.c (ipa_fn_summary_write): Remove inconsistent > > bitfield streaming out. > > > > diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c > > index 2470937460f..31199919405 100644 > > --- a/gcc/ipa-fnsummary.c > > +++ b/gcc/ipa-fnsummary.c > > @@ -4652,7 +4652,6 @@ ipa_fn_summary_write (void) > > info->time.stream_out (ob); > > bp = bitpack_create (ob->main_stream); > > bp_pack_value (&bp, info->inlinable, 1); > > - bp_pack_value (&bp, false, 1); > > bp_pack_value (&bp, info->fp_expressions, 1); > > streamer_write_bitpack (&bp); > > streamer_write_uhwi (ob, vec_safe_length (info->conds));
Hi Richi, Thanks for the review! on 2021/9/17 下午6:04, Richard Biener wrote: > On Fri, Sep 17, 2021 at 12:03 PM Richard Biener > <richard.guenther@gmail.com> wrote: >> >> On Fri, Sep 17, 2021 at 11:43 AM Kewen.Lin <linkw@linux.ibm.com> wrote: >>> >>> Hi, >>> >>> When changing target_info with bitfield, I happened to find this >>> inconsistent streaming in and out. We have the streaming in: >>> >>> bp_pack_value (&bp, info->inlinable, 1); >>> bp_pack_value (&bp, false, 1); >>> bp_pack_value (&bp, info->fp_expressions, 1); >>> >>> while the streaming out: >>> >>> info->inlinable = bp_unpack_value (&bp, 1); >>> info->fp_expressions = bp_unpack_value (&bp, 1) >>> >>> The cleanup of Cilk Plus support seemed to miss to remove the bit >>> streaming out but change with streaming false. >>> >>> By hacking fp_expression_p to return true always, I can see it >>> reads the wrong fp_expressions value (false) out in wpa dumping. >>> >>> Bootstrapped and regress-tested on powerpc64le-linux-gnu Power9. >>> >>> Is it ok for trunk? >> >> OK for trunk and all affected branches (note we need to bump the >> LTO minor version there). The issue comes from the removal >> of cilk+ in r8-4956 which removed the bp_unpack but replaced >> the bp_pack ... >> >> It's a correctness issue as we'll read fp_expressions as always 'false' > > Btw, on branches we could also simply unpack a dummy bit to avoid > changing the format. > Committed in r12-3721. Thanks! As suggested, the patch for branches is listed below. Is ok for branches 9, 10 and 11 after some trunk burn in time? BR, Kewen ----- gcc/ChangeLog: * ipa-fnsummary.c (inline_read_section): Unpack a dummy bit to keep consistent with the side of streaming out. --- diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c index 18bbae145b9..bf635c1f78a 100644 --- a/gcc/ipa-fnsummary.c +++ b/gcc/ipa-fnsummary.c @@ -4403,13 +4403,20 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data, bp = streamer_read_bitpack (&ib); if (info) { - info->inlinable = bp_unpack_value (&bp, 1); - info->fp_expressions = bp_unpack_value (&bp, 1); + info->inlinable = bp_unpack_value (&bp, 1); + /* On the side of streaming out, there is still one bit + streamed out between inlinable and fp_expressions bits, + which was used for cilk+ before but now always false. + To remove the bit packing need to bump LTO minor version, + so unpack a dummy bit here to keep consistent instead. */ + bp_unpack_value (&bp, 1); + info->fp_expressions = bp_unpack_value (&bp, 1); } else { - bp_unpack_value (&bp, 1); - bp_unpack_value (&bp, 1); + bp_unpack_value (&bp, 1); + bp_unpack_value (&bp, 1); + bp_unpack_value (&bp, 1); } count2 = streamer_read_uhwi (&ib);
On Tue, Sep 21, 2021 at 4:09 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > > Hi Richi, > > Thanks for the review! > > on 2021/9/17 下午6:04, Richard Biener wrote: > > On Fri, Sep 17, 2021 at 12:03 PM Richard Biener > > <richard.guenther@gmail.com> wrote: > >> > >> On Fri, Sep 17, 2021 at 11:43 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > >>> > >>> Hi, > >>> > >>> When changing target_info with bitfield, I happened to find this > >>> inconsistent streaming in and out. We have the streaming in: > >>> > >>> bp_pack_value (&bp, info->inlinable, 1); > >>> bp_pack_value (&bp, false, 1); > >>> bp_pack_value (&bp, info->fp_expressions, 1); > >>> > >>> while the streaming out: > >>> > >>> info->inlinable = bp_unpack_value (&bp, 1); > >>> info->fp_expressions = bp_unpack_value (&bp, 1) > >>> > >>> The cleanup of Cilk Plus support seemed to miss to remove the bit > >>> streaming out but change with streaming false. > >>> > >>> By hacking fp_expression_p to return true always, I can see it > >>> reads the wrong fp_expressions value (false) out in wpa dumping. > >>> > >>> Bootstrapped and regress-tested on powerpc64le-linux-gnu Power9. > >>> > >>> Is it ok for trunk? > >> > >> OK for trunk and all affected branches (note we need to bump the > >> LTO minor version there). The issue comes from the removal > >> of cilk+ in r8-4956 which removed the bp_unpack but replaced > >> the bp_pack ... > >> > >> It's a correctness issue as we'll read fp_expressions as always 'false' > > > > Btw, on branches we could also simply unpack a dummy bit to avoid > > changing the format. > > > > Committed in r12-3721. Thanks! > > As suggested, the patch for branches is listed below. > > Is ok for branches 9, 10 and 11 after some trunk burn in time? It's OK for branches without waiting, maybe you can do a LTO bootstrap on the branches for extra safety (just in case we're triggering some hidden issues due to the fix). Thanks, Richard. > BR, > Kewen > ----- > gcc/ChangeLog: > > * ipa-fnsummary.c (inline_read_section): Unpack a dummy bit > to keep consistent with the side of streaming out. > > --- > diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c > index 18bbae145b9..bf635c1f78a 100644 > --- a/gcc/ipa-fnsummary.c > +++ b/gcc/ipa-fnsummary.c > @@ -4403,13 +4403,20 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data, > bp = streamer_read_bitpack (&ib); > if (info) > { > - info->inlinable = bp_unpack_value (&bp, 1); > - info->fp_expressions = bp_unpack_value (&bp, 1); > + info->inlinable = bp_unpack_value (&bp, 1); > + /* On the side of streaming out, there is still one bit > + streamed out between inlinable and fp_expressions bits, > + which was used for cilk+ before but now always false. > + To remove the bit packing need to bump LTO minor version, > + so unpack a dummy bit here to keep consistent instead. */ > + bp_unpack_value (&bp, 1); > + info->fp_expressions = bp_unpack_value (&bp, 1); > } > else > { > - bp_unpack_value (&bp, 1); > - bp_unpack_value (&bp, 1); > + bp_unpack_value (&bp, 1); > + bp_unpack_value (&bp, 1); > + bp_unpack_value (&bp, 1); > } > > count2 = streamer_read_uhwi (&ib); >
on 2021/9/21 下午2:16, Richard Biener wrote: > On Tue, Sep 21, 2021 at 4:09 AM Kewen.Lin <linkw@linux.ibm.com> wrote: >> >> Hi Richi, >> >> Thanks for the review! >> >> on 2021/9/17 下午6:04, Richard Biener wrote: >>> On Fri, Sep 17, 2021 at 12:03 PM Richard Biener >>> <richard.guenther@gmail.com> wrote: >>>> >>>> On Fri, Sep 17, 2021 at 11:43 AM Kewen.Lin <linkw@linux.ibm.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> When changing target_info with bitfield, I happened to find this >>>>> inconsistent streaming in and out. We have the streaming in: >>>>> >>>>> bp_pack_value (&bp, info->inlinable, 1); >>>>> bp_pack_value (&bp, false, 1); >>>>> bp_pack_value (&bp, info->fp_expressions, 1); >>>>> >>>>> while the streaming out: >>>>> >>>>> info->inlinable = bp_unpack_value (&bp, 1); >>>>> info->fp_expressions = bp_unpack_value (&bp, 1) >>>>> >>>>> The cleanup of Cilk Plus support seemed to miss to remove the bit >>>>> streaming out but change with streaming false. >>>>> >>>>> By hacking fp_expression_p to return true always, I can see it >>>>> reads the wrong fp_expressions value (false) out in wpa dumping. >>>>> >>>>> Bootstrapped and regress-tested on powerpc64le-linux-gnu Power9. >>>>> >>>>> Is it ok for trunk? >>>> >>>> OK for trunk and all affected branches (note we need to bump the >>>> LTO minor version there). The issue comes from the removal >>>> of cilk+ in r8-4956 which removed the bp_unpack but replaced >>>> the bp_pack ... >>>> >>>> It's a correctness issue as we'll read fp_expressions as always 'false' >>> >>> Btw, on branches we could also simply unpack a dummy bit to avoid >>> changing the format. >>> >> >> Committed in r12-3721. Thanks! >> >> As suggested, the patch for branches is listed below. >> >> Is ok for branches 9, 10 and 11 after some trunk burn in time? > > It's OK for branches without waiting, maybe you can do a LTO bootstrap > on the branches for extra safety (just in case we're triggering some hidden > issues due to the fix). Thanks! LTO bootstrapped on the branches, separately committed via r11-9024, r10-10146 and r9-9740. BR, Kewen > > Thanks, > Richard. > >> BR, >> Kewen >> ----- >> gcc/ChangeLog: >> >> * ipa-fnsummary.c (inline_read_section): Unpack a dummy bit >> to keep consistent with the side of streaming out. >> >> --- >> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c >> index 18bbae145b9..bf635c1f78a 100644 >> --- a/gcc/ipa-fnsummary.c >> +++ b/gcc/ipa-fnsummary.c >> @@ -4403,13 +4403,20 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data, >> bp = streamer_read_bitpack (&ib); >> if (info) >> { >> - info->inlinable = bp_unpack_value (&bp, 1); >> - info->fp_expressions = bp_unpack_value (&bp, 1); >> + info->inlinable = bp_unpack_value (&bp, 1); >> + /* On the side of streaming out, there is still one bit >> + streamed out between inlinable and fp_expressions bits, >> + which was used for cilk+ before but now always false. >> + To remove the bit packing need to bump LTO minor version, >> + so unpack a dummy bit here to keep consistent instead. */ >> + bp_unpack_value (&bp, 1); >> + info->fp_expressions = bp_unpack_value (&bp, 1); >> } >> else >> { >> - bp_unpack_value (&bp, 1); >> - bp_unpack_value (&bp, 1); >> + bp_unpack_value (&bp, 1); >> + bp_unpack_value (&bp, 1); >> + bp_unpack_value (&bp, 1); >> } >> >> count2 = streamer_read_uhwi (&ib); >>
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c index 2470937460f..31199919405 100644 --- a/gcc/ipa-fnsummary.c +++ b/gcc/ipa-fnsummary.c @@ -4652,7 +4652,6 @@ ipa_fn_summary_write (void) info->time.stream_out (ob); bp = bitpack_create (ob->main_stream); bp_pack_value (&bp, info->inlinable, 1); - bp_pack_value (&bp, false, 1); bp_pack_value (&bp, info->fp_expressions, 1); streamer_write_bitpack (&bp); streamer_write_uhwi (ob, vec_safe_length (info->conds));
Hi, When changing target_info with bitfield, I happened to find this inconsistent streaming in and out. We have the streaming in: bp_pack_value (&bp, info->inlinable, 1); bp_pack_value (&bp, false, 1); bp_pack_value (&bp, info->fp_expressions, 1); while the streaming out: info->inlinable = bp_unpack_value (&bp, 1); info->fp_expressions = bp_unpack_value (&bp, 1) The cleanup of Cilk Plus support seemed to miss to remove the bit streaming out but change with streaming false. By hacking fp_expression_p to return true always, I can see it reads the wrong fp_expressions value (false) out in wpa dumping. Bootstrapped and regress-tested on powerpc64le-linux-gnu Power9. Is it ok for trunk? BR, Kewen ----- gcc/ChangeLog: * ipa-fnsummary.c (ipa_fn_summary_write): Remove inconsistent bitfield streaming out.