[PATCH] D88180: [RFC] HashTree and MachineOutliner HashTree Serialization for cross module data sharing.
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 23 10:45:06 PDT 2020
paquette added a comment.
I think it would make sense to put the StableHashTree implementation in its own patch. Then, in a follow up, you can plumb through the outliner support.
The data structure itself needs tests outside of the outliner, and I think it references the outliner itself a little too much in the comments.
I also feel like the outliner shouldn't be responsible for producing HashTree. That seems like a different thing which may have its own cost model and considerations. It might make sense to adapt the IRSimilarity framework to MIR and use that for the purposes of producing the tree.
Having the outliner consume the tree seems fine to me though.
================
Comment at: llvm/include/llvm/CodeGen/MachineOutliner.h:185
+ /// Tells if there is an instance of this OutlinedFunction that is outlined in
+ /// another module. DoesSequenceMatchesOffModule helps getBenefit() to
+ /// consider one additional candidate match that may exists outside of the
----------------
Variable name in comment should match the actual variable name.
(Included existing typo for clarity)
================
Comment at: llvm/include/llvm/CodeGen/MachineOutliner.h:188
+ /// current module.
+ bool DoesSequenceMatcheOffModuleCandidate = false;
+
----------------
- Should say "Matches" or "Match", not "Matche"
- Maybe a more succinct name?
================
Comment at: llvm/include/llvm/CodeGen/MachineOutliner.h:190
+
+ /// The sequence of stable_hash'es for a Candidate in Candidates.
+ /// StableHashSequence is empty if computing hashes is disabled or if
----------------
Typo
================
Comment at: llvm/include/llvm/CodeGen/MachineOutliner.h:194
+ /// is not able have a stable_hash computed.
+ std::vector<stable_hash> StableHashSequence;
+
----------------
If you use an `Optional`, you can differentiate between "it's just empty" and "it's not actually being used" in the type itself.
Also, would it make sense to use a `SmallVector` here?
http://llvm.org/docs/ProgrammersManual.html#vector
> However, SmallVector<T, 0> is often a better option due to the advantages listed [in the SmallVector section]. std::vector is still useful when you need to store more than UINT32_MAX elements or when interfacing with code that expects vectors
================
Comment at: llvm/include/llvm/CodeGen/MachineStableHash.h:1
//===------------ MIRVRegNamerUtils.h - MIR VReg Renaming Utilities -------===//
//
----------------
Might be worth fixing the filename here in a NFC commit?
================
Comment at: llvm/include/llvm/CodeGen/MachineStableHash.h:25
stable_hash stableHashValue(const MachineOperand &MO);
+
stable_hash stableHashValue(const MachineInstr &MI, bool HashVRegs = false,
----------------
remove unnecessary whitespace change
================
Comment at: llvm/include/llvm/CodeGen/MachineStableHash.h:31
+std::vector<stable_hash>
+stableHashMachineInstrs(MachineBasicBlock::iterator &Begin,
+ const MachineBasicBlock::iterator &End);
----------------
Why not both const?
================
Comment at: llvm/include/llvm/CodeGen/MachineStableHash.h:31
+std::vector<stable_hash>
+stableHashMachineInstrs(MachineBasicBlock::iterator &Begin,
+ const MachineBasicBlock::iterator &End);
----------------
paquette wrote:
> Why not both const?
Probably should have a documentation comment?
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:11
+/// Contains a stable hash tree implementation based on llvm::stable_hash.
+/// Primarily used or Global Machine Outlining but is reusable.
+///
----------------
I think that this comment can describe what this actually does in a bit more detail. If this is intended to be a reusable data structure (as the comment implies), I think it'd make sense to address the following questions:
- What does the stable hash tree actually do?
- Why would you use it in a transformation?
Also "Global Machine Outlining" isn't defined anywhere.
In the patch description you have:
> A lot of this work is inspired by or directly refactored from the Global Machine Outliner for ThinLTO talk from EuroLLVM 2020 (https://llvm.org/devmtg/2020-04/talks.html#TechTalk_58).
So it'd be nice to include that somewhere, so people curious about that can take a look.
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:28
+
+// A node in the hash tree might be terminal, i.e. it represents the end
+// of an stable instruction hash sequence that was outlined in some module.
----------------
- Should be a Doxygen comment
- Try to use class names to make things clear
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:33
+//
+// Data is the Hash for the current node
+// IsTerminal is true if this node is the last node in a hash sequence
----------------
Move member variable documentation into the struct.
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:34
+// Data is the Hash for the current node
+// IsTerminal is true if this node is the last node in a hash sequence
+struct HashNode {
----------------
Move member variable documentation into the struct.
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:36
+struct HashNode {
+ stable_hash Data = 0LL;
+ bool IsTerminal{false};
----------------
If you use a more meaningful name than "Data", it shouldn't be necessary to document this?
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:37
+ stable_hash Data = 0LL;
+ bool IsTerminal{false};
+ std::unordered_map<stable_hash, std::unique_ptr<HashNode>> Successors;
----------------
Could this just be a function that checks if the map is empty?
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:38
+ bool IsTerminal{false};
+ std::unordered_map<stable_hash, std::unique_ptr<HashNode>> Successors;
+};
----------------
Would it be appropriate to use an `IndexedMap` here?
http://llvm.org/docs/ProgrammersManual.html#llvm-adt-indexedmap-h
> IndexedMap is a specialized container for mapping small dense integers (or values that can be mapped to small dense integers) to some other type. It is internally implemented as a vector with a mapping function that maps the keys to the dense integer range.
I suppose it depends on if `stable_hash` tends to be dense, by whatever measure of dense is being used here.
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:41
+
+class HashTree {
+public:
----------------
Match the name of the file?
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:45
+ /// edges and CallbackVertex for the vertices with the stable_hash for the
+ /// source and the stable_hash of the sink for the edge. Using walkEdges it
+ /// should be possible to traverse the HashTree and serialize it, compute its
----------------
I think that you can drop the part about `walkEdges`?
General documentation for the data structure and how it works would be better in the `\file` comment at the top.
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:49
+ void walkGraph(
+ std::function<void(const HashNode *, const HashNode *)> CallbackEdge,
+ std::function<void(const HashNode *)> CallbackVertex) const;
----------------
These type names are pretty long.
Maybe it'd be good to reduce the cognitive overload by doing something like this somewhere:
```
/// Graph traversal callback types.
///{
using EdgeCallbackFn = std::function<void(const HashNode *, const HashNode *)>;
using NodeCallbackFn = std::function<void(const HashNode *)>;
///}
```
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:50
+ std::function<void(const HashNode *, const HashNode *)> CallbackEdge,
+ std::function<void(const HashNode *)> CallbackVertex) const;
+
----------------
Although vertex and node are interchangeable terms, I think it'd be good to be consistent and just choose one?
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:56
+
+ // Walks the vertices of a HashTree using walkGraph.
+ void walkVertices(std::function<void(const HashNode *)> Callback) const;
----------------
Should be Doxygen
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:59
+
+ /// Uses HashTree::walkEdges to print the edges of the hash tree.
+ /// If a DebugMap is provided, then it will be used to provide richer output.
----------------
Documentation comments don't need to include implementation info; that can go out of date.
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:60
+ /// Uses HashTree::walkEdges to print the edges of the hash tree.
+ /// If a DebugMap is provided, then it will be used to provide richer output.
+ void dump(raw_ostream &OS = llvm::errs(),
----------------
Use Doxygen stuff
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:70
+
+ /// When building a hash tree, insert sequences of stable instruction hashes.
+ void insertIntoHashTree(
----------------
No need to mention where this is called if you document the algorithm somewhere.
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:74
+
+ // When using a hash tree, starting from the root, check whether a sequence
+ // of stable instruction hashes ends up at a terminal node.
----------------
- Documentation should just say what the function does, not include implementation details.
- Doxygen comment.
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:79
+private:
+ // The hash tree is a compact representation of the set of all outlined
+ // instruction sequences across all modules.
----------------
Probably good to not mention outlining here if this is supposed to be general-purpose?
================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:89
+
+ void insertIntoHashTree(const std::vector<stable_hash> &StableHashSequence);
+};
----------------
Needs documentation.
================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:114
+ "of the candidate sequences in a HashTree and write the tree to "
+ "disk. In read mode, the outliner will read a provided HashTree "
+ "from disk and use the tree to aid in adjusting the threshold for "
----------------
This is a long sentence. Split it up?
================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:372
+ /// stable-hash of the outlined instruction sequence.
+ HashTree OutlinerHashTree;
----------------
Capitalization?
================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:596
+ std::vector<stable_hash> StableHashSequence;
+ bool IsCandidateInHashTree = false;
----------------
Can this go in a function?
================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:611
+ for (auto I = CandidateBegin, E = CandidateEnd; I != E; ++I) {
+ llvm::dbgs() << "# MI: ";
+ I->dump();
----------------
Do you have to use `llvm::` here?
================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:629
+ // a single profitable candidate to be outlined. When we have a serialized
+ // hash tree, there is profitability in outling a Candidate which matches
+ // the HashTree because outlined functions can be merged with linkonceodr.
----------------
================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:668
+ // candidate this breaks down.
+ if (IsSingleCandidateInHashTree)
+ OF.Candidates.pop_back();
----------------
I'm not a fan of messing with the found candidates or cost model to make this work.
If you wanted to handle candidates that appear once across many modules, I think it would be best to pre-populate a hash tree with known beneficial candidates versus trying to guess/mess with stuff during outlining?
Since the tree is serialized to JSON, it should be possible to pre-populate it without using the outliner...
Maybe it'd make sense to adapt the IR similarity framework for this portion somehow versus putting all of this in the outliner? It seems like generating the hash tree is really outside the scope of the pass. Consuming and using it seems okay though.
================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:848
+ if (OF.StableHashSequence.size()) {
+ ModuleStableHashSequences.push_back(OF.StableHashSequence);
----------------
remove braces
================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:946
+ if (StringRef(OutlinerHashTreeMode).lower() == "write" &&
+ ModuleStableHashSequences.size()) {
+ OutlinerHashTree.insertIntoHashTree(ModuleStableHashSequences);
----------------
remove braces
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88180/new/
https://reviews.llvm.org/D88180
More information about the llvm-commits
mailing list