[PATCH] D19404: Reorganize GlobalValueSummary with a "Flags" bitfield.

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

In http://reviews.llvm.org/D19404#409074, @tejohnson wrote:

> I suppose the advantage of encoding all of these flags into a single "flags" bitcode field (rather than having separate fixed bitcode fields for each flag) is twofold:
>
> 1. It is easier to add new flags in the future (requires touching less places in the code)
> 2. They can be compressed with the VBR encoding (assuming most individual flags are "off"/0). The disadvantages are:
> 3. The bitcode record doesn't document the supported flags
> 4. Harder to distinguish old bitcode files that are missing any new flag altogether from newer bitcode that has the flags but they are "off"/0.
>
>   The biggest potential issue seems to me the second disadvantage. Particularly if the a 0 value for the new flag allows us to be more aggressive in certain cases than we were with the older bitcode version that didn't have the flag. Any thoughts on how we could mitigate this risk? Otherwise, it seems like it might be more robust to explicitly list each flag in the bitcode format.


You're right. We need to version the record somehow.
Even with separate fields with have the issue. Usually we can work around it with separate field because the number of entries in the record tells us which "version" we have. Here we can't do that for every record because some record have variable-length entries.
Something that could work is to have `[ ..., FlagsVersion (VBR6), Flags (VBR6), ...]` with `FlagsVersion` an integer incremented every time we make a change to the `Flags` content.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:704
@@ +703,3 @@
+static GlobalValueSummary::GVFlags getDecodedGVSummaryFlags(uint64_t RawFlags) {
+  // Linkage don't need upgrade at that time for the summary. Any future change
+  // to the getDecodedLinkage() function will need to be taken into account here
----------------
tejohnson wrote:
> Don't understand the first sentence, can you clarify what that is saying?
> 
> Oh, I think what you are saying is that we don't need to do getDecodeLinkage since we aren't (yet) supporting any legacy linkage encodings? Could you clarify?
Yes, ThinLTO is too recent so there is no summary that needs auto-upgrade.
(I think I can already submit a separate patch that does this change for the current representation of the summary)

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:539
@@ +538,3 @@
+  uint64_t RawFlags = 0;
+  // Linkage don't need to be remapped at that time for the summary. Any future
+  // change to the getEncodedLinkage() function will need to be taken into
----------------
tejohnson wrote:
> Similarly didn't at first understand the first sentence here - needs a similar clarification.
OK


http://reviews.llvm.org/D19404