[PATCH] D88180: [RFC] StableHashTree Implementation.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

If this data structure is a trie, is there any reason you can't just improve the existing SuffixTree to do all of this?

A suffix tree is just a compressed trie.



================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:58
+  stable_hash Hash = 0LL;
+  bool IsTerminal{false};
+  std::unordered_map<stable_hash, std::unique_ptr<HashNode>> Successors;
----------------
Should `IsTerminal` ever be modified outside of `Insert` and `readFromBuffer`?

Would it make sense to have it be a private member with a getter?


================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:101
+  /// Serializes a StableHashTree from a file at \p Filename.
+  llvm::Error readFromFile(StringRef Filename) {
+    llvm::SmallString<256> Filepath(Filename);
----------------
Should this be in StableHashTree.cpp?


================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:123
+
+  void insert(const std::vector<StableHashSequence> &Sequences) {
+    for (const auto &Sequence : Sequences)
----------------
Needs documentation?

What happens if something inserted was already in the tree?


================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:130
+  /// otherwise.
+  bool find(const StableHashSequence &Sequence) const;
+
----------------
Seems like this may be a bit nicer as an Optional which returns where the thing was found?

```
Optional<HashNode *> find(const StableHashSequence &Sequence) const;
```

That way you can also access the specific node if you need it.

Or, even better, if you had an iterator type for this data structure you could do something like this:

```
iterator find(const StableHashSequence &Sequence) const;
```

I feel like the iterator idea is probably better, since it's more consistent with the rest of the LLVM data types. You could even have an `edge_iterator` and a `vertex_iterator` for edge/vertex walks.



================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:136
+  /// StableHashTree).
+  size_t size(bool GetTerminalCountOnly = false) const {
+    size_t Size = 0;
----------------
Should these functions be in StableHashTree.cpp?


================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:144
+
+  size_t depth() const {
+    size_t Size = 0;
----------------
Missing comment?


================
Comment at: llvm/lib/CodeGen/StableHashTree.cpp:68
+    assert(Index = NodeMap.size() + 1 &&
+                   "Expected size of ModeMap to increment by 1");
+  });
----------------
Typo


================
Comment at: llvm/lib/CodeGen/StableHashTree.cpp:117
+  if (!JO)
+    return llvm::createStringError(std::error_code(), "Bad Json");
+
----------------
Should be consistent with how JSON is capitalized in things the a person might see on the command line.


================
Comment at: llvm/lib/CodeGen/StableHashTree.cpp:181
+    auto I = Current->Successors.find(StableHash);
+    if (I == Current->Successors.end()) {
+      std::unique_ptr<HashNode> Next = std::make_unique<HashNode>();
----------------
Nit: It's a bit nicer to read if you put the simpler situations as the one you continue from. This lessens the indentation level of the more complicated case:

```
if (I != Current->Successors.end()) {
  Current = I->second.get();
  continue;
}

// Didn't find the hash in the current node's successors. Create a new one.
std::unique_ptr<HashNode> Next = std::make_unique<HashNode>();
// ... 

```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88180/new/

https://reviews.llvm.org/D88180