Page MenuHomePhabricator

[Assembler] Emit summary index flags
ClosedPublic

Authored by evgeny777 on Feb 11 2020, 9:20 AM.

Diff Detail

Event Timeline

evgeny777 created this revision.Feb 11 2020, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2020, 9:20 AM
tejohnson added inline comments.Feb 12 2020, 6:46 AM
llvm/lib/IR/AsmWriter.cpp
754

Looking at SlotTracker::processIndex, I think it could be improved and we can compute this there. For clarity, that method should probably iterate the typeIdCompatibleVtableMap before setting TypeIdNext (although the typeIdCompatibleVtable and the typeId summaries are mutually exclusive, I think it would be clearer to do it in that order). Then we could add something like a "IndexSlot" variable that can be set to TypeIdNext at the end. I think that will make it more clear what needs to change if we later need to add an additional type of slot.

2746

The EnableSplitLTOUnit flag will be set on per-module summaries. In which case I think this does the right thing, since it will be non-zero. But the comment seems wrong.

evgeny777 updated this revision to Diff 244659.Feb 14 2020, 7:34 AM

Addressed comments

Looking at SlotTracker::processIndex, I think it could be improved and we can compute this there. For clarity, that method should probably iterate the typeIdCompatibleVtableMap before setting TypeIdNext (although the typeIdCompatibleVtable and the typeId summaries are mutually exclusive, I think it would be clearer to do it in that order). Then we could add something like a "IndexSlot" variable that can be set to TypeIdNext at the end.

Makes sense but I don't think we need an extra variable. We can simply use TypeIdNext, as it will be equal to number of slots in both combined and per-module summaries.

This revision is now accepted and ready to land.Feb 14 2020, 8:28 AM
This revision was automatically updated to reflect the committed changes.