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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

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:

1. The bitcode record doesn't document the supported flags
2. 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.


================
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
----------------
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?

================
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
----------------
Similarly didn't at first understand the first sentence here - needs a similar clarification.


http://reviews.llvm.org/D19404