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.guent...@gmail.com> wrote: >> >> On Fri, Sep 17, 2021 at 11:43 AM Kewen.Lin <li...@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);