This is an archive of the discontinued LLVM Phabricator instance.

[llvm][Bitcode] Add bitcode reader/writer for DSOLocalEquivalent
ClosedPublic

Authored by leonardchan on Feb 5 2021, 12:27 PM.

Details

Summary

This is necessary for compilation with [thin]lto.

Diff Detail

Event Timeline

leonardchan created this revision.Feb 5 2021, 12:27 PM
leonardchan requested review of this revision.Feb 5 2021, 12:27 PM

*ping* Any comments for this patch? I believe these are mostly mechanical changes that were left out of the original DSOLocalEquivalent patch.

I've never patched the bitcode reader/writer before, I think, but from similar patches I've seen, this look good to me. Is there any sort of registration you're supposed to do of the new code?

I've never patched the bitcode reader/writer before, I think, but from similar patches I've seen, this look good to me. Is there any sort of registration you're supposed to do of the new code?

Hmm. By registration, do you mean making cmake aware of the new test? I don't *think* there's anything else I may need to change. I mainly just copied similar logic from how the other constant codes are handled.

Adding @rnk who may have more familiarity.

leonardchan removed a subscriber: rnk.

I've never patched the bitcode reader/writer before, I think, but from similar patches I've seen, this look good to me. Is there any sort of registration you're supposed to do of the new code?

Hmm. By registration, do you mean making cmake aware of the new test? I don't *think* there's anything else I may need to change. I mainly just copied similar logic from how the other constant codes are handled.

Adding @rnk who may have more familiarity.

Some of the other serializers using bitcode require some of the codes to be registered, either as abbreviations or just for incidental reasons. If there's nothing like that for similar record codes, though, it's probably not a thing for BC.

I've never patched the bitcode reader/writer before, I think, but from similar patches I've seen, this look good to me. Is there any sort of registration you're supposed to do of the new code?

Hmm. By registration, do you mean making cmake aware of the new test? I don't *think* there's anything else I may need to change. I mainly just copied similar logic from how the other constant codes are handled.

Adding @rnk who may have more familiarity.

Some of the other serializers using bitcode require some of the codes to be registered, either as abbreviations or just for incidental reasons. If there's nothing like that for similar record codes, though, it's probably not a thing for BC.

Ah I see. So something similar to:

Abbv->Add(BitCodeAbbrevOp(bitc::CST_CODE_STRING));

It seems there's a handful of other ConstantCode BCs that use this, but most of them don't seem to use it. I'm guessing this might also be a case where it's not required.

phosek accepted this revision.Feb 18 2021, 2:40 PM

LGTM

This revision is now accepted and ready to land.Feb 18 2021, 2:40 PM
This revision was landed with ongoing or failed builds.Feb 22 2021, 10:39 AM
This revision was automatically updated to reflect the committed changes.