[PATCH] D26872: Outliner: Add MIR-level outlining pass
Sean Silva via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 10 04:34:19 PST 2017
silvas added a comment.
Great work!
This round of review feedback is mostly about comment accuracy and some implementation improvements like centralizing/encapsulating the numbering state machine, handling the EndIdx for leaf nodes in a simpler way, and avoiding copying too many vectors around. Also various style/readability nits.
The testing still seems light considering how much functionality this adds. Can we use MIR to test this? Ideally we'd have pretty good coverage of X86InstrInfo::isLegalToOutline
One way to think about this is: suppose we run this on a bunch of programs in the wild and find some bugs in the suffix tree construction algorithm or isLegalToOutline. How are we going to write tests for those fixes?
Since you've done such a nice job keeping SuffixTree separate (with the simple interface based on `unsigned`) it might even be possible to write a C++ unittest for it. That requires pulling it up into a header though and other headaches/boilerplate that might not be worth it right now.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:70
+
+/// Stores instruction-integer mappings for MachineBasicBlocks in the program.
+///
----------------
My reading of this comment interprets this as saying that this class e.g. holds a DenseMap of MI's to integers itself, but that is not the case. Can you make this more precise?
The term "mapping" is somewhat confusing since in common usage a "mapping" usually denotes a container. But throughout this patch the term "mapping" is used to refer to a "string". I think I get what sense it is meant in (e.g. "this is the mapping of this MBB through the our value numbering map"). I can't think of any really good names except for something horribly verbose like "StringOfInstructionNumbers" or something like that. Anyway, you probably want to beef up this comment to describe the sense in which "mapping" is used here and throughout this patch.
After staring at the patch for a while the term "mapping" has grown on me, so it's not a big deal.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:72
+///
+/// This is used for compatability with the suffix tree. Mappings will tend to
+/// be referred to as strings from the context of the suffix tree.
----------------
Rather than a generic "this is used for compatibility with the suffix tree", can you be more precise. Something like "Our suffix tree implementation operates on this class" or something? After changing the constructor argument of SuffixTree to `const std::vector<std::vector<unsigned>> &` the only use of this class is as an internal data structure of the SuffixTree, which is an even more precise statement.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:76
+/// In the outliner, each \p MachineBasicBlock in the program is mapped to a
+/// \p vector of \p unsigneds. Each \p unsigned is either the hash for an
+/// instruction, or unique. Unique unsigneds represent instructions that the
----------------
This description in terms of "hash" vs "unique" doesn't seem accurate. Once you pull out a class to encapsulate the numbering you can just mention that here. As far as users of this class are concerned the numbers are just unique symbols; I don't think you need to go into too much detail besides linking to the place where we assign the numbering.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:95
+/// that vector as flattened indices. The purpose of the \p ProgramMapping
+/// is to let us pretend a 2D vector is a flattened one. We can then place the
+/// \p ProgramMapping in the \p SuffixTree, find outlining candidates, but
----------------
Can you add a high-level explanation of why we have a 2D vector in the first place. I.e. why do we need to "pretend" instead of just materializing the flattened vector?
One thing that may help make this clearer is encapsulating the mutation of `MBBMappings` a bit better. Right now there seem to be some un-encapsulated mutations, so just looking at the class it's not clear what the elementary operations on it are.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:99
+/// need to remember this because we need to clone the instructions from somewhere.
+struct ProgramMapping {
+
----------------
Generally the term "program" is reserved for talking about the final linked executable, but this code (except during LTO) generally operates on a single Module/TU. Can you give this a better name? Maybe just `BlockMappings` or something like that.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:122
+ size_t NumMappings = MBBMappings.size();
+ for (MappingIdx = 0; MappingIdx < NumMappings; MappingIdx++) {
+
----------------
It shouldn't be too hard to bring this down to log(N) if necessary, but it surprises me that O(N) is fine when N = number of MBB's in the module (for reference, a typical FullLTO for a codebase ~ the size of clang has 10's of thousands of functions, and probably an order of magnitude more MBB's). Can you add a comment explaining that it takes linear time and why that's fine (or not fine, but fine for now)?
================
Comment at: lib/CodeGen/MachineOutliner.cpp:143
+
+ /// Returns the element of \p ProgramMapping as a 2D mapping at \p QueryIdx.
+ unsigned elementAt(size_t QueryIdx) {
----------------
Why is this comment talking about "2D mapping"? There is just a single index.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:202
+
+ /// \brief For internal nodes, a pointer to the internal node representing
+ /// the same mapping with the first character chopped off.
----------------
This comment is pure gold. Nice.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:241
+ if (StartIdx != EmptyIdx)
+ SubstringLen = *EndIdx - StartIdx + 1;
+
----------------
Comment on the `+ 1`.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:264
+///
+/// Suffix trees contain the suffixes of their input strings in their leaves.
+/// This property makes it possible to quickly determine long repeated
----------------
Isn't it a more like that the leaves "represent" suffixes rather than "contain" suffixes?
================
Comment at: lib/CodeGen/MachineOutliner.cpp:266
+/// This property makes it possible to quickly determine long repeated
+/// substrings of strings.
+///
----------------
I don't think this is correct. It isn't the leaves representing suffixes per se that facilitates finding repeated substrings, but rather the fact that the internal nodes represent repeated substrings (shared prefixes of the suffixes). You may want to beef up this paragraph on suffix trees a bit to describe the basic invariants a bit more (leaves represent suffixes, internal nodes represent shared prefixes of the suffixes).
================
Comment at: lib/CodeGen/MachineOutliner.cpp:269
+/// In this implementation, a "string" is a vector of unsigned integers.
+/// These integers may result from hashing some data type. A suffix tree can
+/// contain 1 or many strings, which can then be queried as one large string.
----------------
Same comment as on ProgramMapping. The integers themselves aren't hashes (otherwise this code would have to do something special for collisions).
================
Comment at: lib/CodeGen/MachineOutliner.cpp:279
+///
+/// Note that despite the main structure being a tree, the implementation
+/// of the suffix tree really forms a digraph due to the suffix links
----------------
I'm not sure what you are trying to convey with this paragraph. Maybe you can just mention that the implementation maintains parent links (maybe also describe what they are used for). I don't see the big picture for talking about cycles or explicit digraphs here.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:288
+ ///
+ /// Note that because this is a bump pointer allocator, we don't have to
+ /// manually delete the nodes in the tree.
----------------
The RAII behavior of BumpPtrAllocator is pretty well-known, so you don't need to explicitly mention it (that's the beauty of RAII; it just cleans up for you!). This comment can probably be reduced to "Allocator that owns all nodes in the tree".
================
Comment at: lib/CodeGen/MachineOutliner.cpp:292
+
+ /// Maintains the end indices of the internal nodes in the tree.
+ ///
----------------
This arrangement of adding an extra layer of indirection for the special "EndIdx" handling for leaf nodes is interesting but after staring at the code for a while it seems like it obscures things (or maybe I'm missing something).
It seems that the only thing that needs this extra layer of indirection is during construction in the test `if (StrIdx > *(CurrSuffixTreeNode->EndIdx)) {` which could be replaced by something like `if (StrIdx > (CurrSuffixTreeNode->EndIdx == -1 : LeafEndIdx : CurrSuffixTreeNode->EndIdx) {` or similar. To do that, SuffixTreeNode::EndIdx would just be an integer held by value, with -1 being a sentinel indicating that this is a leaf that needs the special handling in that one test.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:294
+ ///
+ /// Each internal node is guaranteed to never have its end index change
+ /// during the construction algorithm; however, leaves must be updated at
----------------
what does the EndIdx even mean for an internal node, since they can be shared? Maybe explain that in the comment of the EndIdx member of SuffixTreeNode?
================
Comment at: lib/CodeGen/MachineOutliner.cpp:302
+ /// manually delete the end indices of the nodes in the tree.
+ BumpPtrAllocator InternalEndIdxAllocator;
+
----------------
You can use the same BumpPtrAllocator for both of these if you want. It's kind of nice to have a place for these comments though so no biggie.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:347
+ N = new (NodeAllocator) SuffixTreeNode(StartIdx, E, Root);
+ N->Parent = Parent;
+
----------------
Can `Parent` just be a constructor argument?
================
Comment at: lib/CodeGen/MachineOutliner.cpp:366
+
+ assert(ChildPair.second && "Node has a null child!");
+
----------------
This assert is inside an `if(ChildPair.second != nullptr)` so probably doesn't buy you much.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:547
+ // tree
+ else if (N.SuffixIdx != EmptyIdx && MaxLen < (LabelHeight - N.size())) {
+ MaxLen = LabelHeight - N.size();
----------------
Nit: move the comment inside so you can use the coding-standard compliant `} else if (...) {`
================
Comment at: lib/CodeGen/MachineOutliner.cpp:595
+ // empty string.
+ if (CurrSuffixTreeNode->Children[QueryString[CurrIdx]] != nullptr &&
+ CurrSuffixTreeNode->Children[QueryString[CurrIdx]]->IsInTree) {
----------------
Nit: Putting this `CurrSuffixTreeNode->Children[QueryString[CurrIdx]]` expression (used 3 times) in a variable will make things a bit shorter and also give you an opportunity to give that value a name. E.g. maybe `if (Child && Child->IsInTree) {`?
================
Comment at: lib/CodeGen/MachineOutliner.cpp:604
+
+ // The node represents a non-empty string, so we should match against it and
+ // check its children if necessary.
----------------
Nit: move this comment inside the `else` so that you have `} else {`.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:613
+ // Found and possibly break based off of the case we find.
+ while (CurrIdx < QueryString.size() - 1) {
+
----------------
Comment on the `- 1` part of this. It's setting off my off-by-one error spidey sense.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:673
+ size_t Len = 0;
+ std::vector<std::pair<std::vector<unsigned>, size_t>> Occurrences;
+ SuffixTreeNode *N = findString(QueryString, Len, Root);
----------------
You're copying quite a few std::vector's here. Can they be ArrayRef's?
================
Comment at: lib/CodeGen/MachineOutliner.cpp:725
+ /// \brief Create a suffix tree from a list of strings \p Strings, treating
+ /// that list as a flat string.
+ SuffixTree(const ProgramMapping &Strings) {
----------------
This iterates over Strings.MBBMappings. In what sense does it treat Strings as "flat" if it looks at individual substrings?
Also, I find it a bit weird that we take a ProgramMapping as input to this constructor, but then all these `append` calls seem to build up a different ProgramMapping. Do the two ProgramMapping's end up being equivalent? It would be nice to see a bit more explanation about this. At least for the purposes of this constructor, maybe a `const std::vector<std::vector<unsigned>> &` is the natural interface because it doesn't use any of the fancy methods on ProgramMapping.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:751
+ /// in the \p ProgramMapping.
+ size_t FlatMappingStartIdx;
+
----------------
It seems a bit weird to me that this class is caring explicitly about the distinction between the "flat" and non-"flat" senses of ProgramMapping. I thought that ProgramMapping was just supposed to encapsulate a 2D ragged array and make it look flat, but here it seems that the external code still cares about the distinction between 2D-ness and flat-ness. Can you make it a bit clearer in the code and comments what ProgramMapping is supposed to represent and what its interactions with the rest of the code are?
================
Comment at: lib/CodeGen/MachineOutliner.cpp:797
+
+ /// The number this function will be given in the \p ProgramMapping.
+ size_t Id;
----------------
I don't see much mention of the function Id's in `ProgramMapping`. Looking at the code, it seems like it should be `unsigned` and roughly represents the call instruction that jumps to the outlined function.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:826
+ DenseMap<MachineInstr *, unsigned, MachineInstrExpressionTrait>
+ InstructionIntegerMap;
+
----------------
There are a couple members here related to the legal-instruction/illegal-instruction/function numbering that could stand to be pulled out into an isolated class (which can then be held by value in the pass) separate from the pass boilerplate. Such a class will also be a good place to authoritatively document the numbering scheme and encapsulate it.
================
Comment at: lib/CodeGen/MachineOutliner.cpp:943
+ // it the next available one.
+ auto I = InstructionIntegerMap.insert(
+ std::make_pair(&MI, CurrLegalInstrMapping));
----------------
Small readability nit: use `std::tie(I, WasInserted) = ...` or something like that to make this a bit clearer (this map interface returning the pair is always confusing without that).
================
Comment at: lib/CodeGen/MachineOutliner.cpp:1154
+ // Remove the candidate from the mapping in the suffix tree first, and
+ // replace it with the associated function's id.
+ // auto Begin = Mapping.MBBMappings[C.IdxOfMBB]->begin() + C.StartIdxInMBB;
----------------
It isn't really the "function's id" but rather the id for a call instruction that jumps to it, right?
================
Comment at: lib/CodeGen/MachineOutliner.cpp:1155
+ // replace it with the associated function's id.
+ // auto Begin = Mapping.MBBMappings[C.IdxOfMBB]->begin() + C.StartIdxInMBB;
+ auto Begin = Mapping.MBBMappings[C.IdxOfMBB].begin() + C.StartIdxInMBB;
----------------
Nit: remove commented out code.
Repository:
rL LLVM
https://reviews.llvm.org/D26872
More information about the llvm-commits
mailing list