This is an archive of the discontinued LLVM Phabricator instance.

Reorganize GlobalValueSummary with a "Flags" bitfield.
ClosedPublic

Authored by mehdi_amini on Apr 21 2016, 11:52 PM.

Details

Summary

Right now it only contains the LinkageType, but will be extended
with "hasSection", "isOptSize", "hasInlineAssembly", etc.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Reorganize GlobalValueSummary with a "Flags" bitfield..
mehdi_amini updated this object.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini added a subscriber: llvm-commits.

remove debug

tejohnson edited edge metadata.Apr 22 2016, 8:50 AM

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.

lib/Bitcode/Reader/BitcodeReader.cpp
723

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?

lib/Bitcode/Writer/BitcodeWriter.cpp
792

Similarly didn't at first understand the first sentence here - needs a similar clarification.

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.

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.

lib/Bitcode/Reader/BitcodeReader.cpp
723

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)

lib/Bitcode/Writer/BitcodeWriter.cpp
792

OK

In D19404#409351, @joker.eph wrote:

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.

I like the version idea. However, instead of bloating out every single summary with this info, since all summaries should have the same version, I think we should put a single summary version id record at the start the summary section (not specific to the flags, since we will want to use this to detect other summary changes too).

Indeed, great suggestion!

But now that I think about it, we have a global "epoch" number that I added recently in the bitcode (see IDENTIFICATION_BLOCK_ID). We could bump it when needed.

In D19404#409415, @joker.eph wrote:

Indeed, great suggestion!

But now that I think about it, we have a global "epoch" number that I added recently in the bitcode (see IDENTIFICATION_BLOCK_ID). We could bump it when needed.

If we use this it will need to be emitted into the combined index as well (it isn't currently - easy enough to do).

The only issue with that is that I would expect more "churn" at least for awhile with the summary format, not sure if it is a big deal to have the epoch bump frequently due to that. Otherwise seems fine to me.

mehdi_amini marked 4 inline comments as done.
mehdi_amini edited edge metadata.

Rebase and address comments

rebase, address comments.

tejohnson accepted this revision.Apr 23 2016, 8:10 PM
tejohnson edited edge metadata.

One suggestion for comment below.

lib/Bitcode/Reader/BitcodeReader.cpp
724

Like the new comment. Could you put the same thing in getEncodedGVSummaryFlags?

This revision is now accepted and ready to land.Apr 23 2016, 8:10 PM