[PATCH] D88180: [RFC] StableHashTree Implementation.

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 20 inline comments as done.
plotfi added inline comments.


================
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;
----------------
paquette wrote:
> Could this just be a function that checks if the map is empty?
I thought this myself, but you need an IsTerminal flag because you could have some sequence you want to hash like:

```
    ORRWri
    ORRWri
    ORRWri
    RET
```

as well as

```
    ORRWri
    ORRWri
    ORRWri
```

You need the terminal to know that even though you have a node with successors that that know can be the terminal node in a sequence that was added. 


================
Comment at: llvm/include/llvm/CodeGen/StableHashTree.h:38
+  bool IsTerminal{false};
+  std::unordered_map<stable_hash, std::unique_ptr<HashNode>> Successors;
+};
----------------
paquette wrote:
> 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.
Can I add this as a post commit NFC commit? I am unsure on the tradeoffs here at the moment. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88180