[PATCH] D92674: [TableGen] Cache the vectors of records returned by getAllDerivedDefinitions().

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/TableGen/Record.cpp:2602
+    StringRef ClassName) const {
+  std::string ClassNameString = std::string(ClassName);
+  auto It = ClassRecordsMap.find(ClassNameString);
----------------
I'd probably write this as `std::string ClassNameString = ClassName.str()` but could be written as `std::string ClassNameString(ClassName);` as well, if you prefer, but as-written (`std::string ClassNameString = std::string(ClassName);`) it's a bit overly verbose/redundant.


================
Comment at: llvm/lib/TableGen/Record.cpp:2602-2610
+  std::string ClassNameString = std::string(ClassName);
+  auto It = ClassRecordsMap.find(ClassNameString);
+  if (It != ClassRecordsMap.end()) {
+    return It->second;
+  }
+
+  auto Records = getAllDerivedDefinitions(makeArrayRef(ClassName));
----------------
dblaikie wrote:
> I'd probably write this as `std::string ClassNameString = ClassName.str()` but could be written as `std::string ClassNameString(ClassName);` as well, if you prefer, but as-written (`std::string ClassNameString = std::string(ClassName);`) it's a bit overly verbose/redundant.
This causes a few redundant map lookups on the expensive path-, but probably the overall best solution would be using StringMap for this anyway (no need to do conversions via std::string in that case, for instance)
```
auto p = m.try_emplace(ClassNameString);
if (p.second) {
  p.first->second = getAllDerivedDefinitions(makeArrayRef(ClassName);
}
return p.first->second;

```

If this continues to use std::unordered_map, the code should be the same except for the need to call "str()" on the use of ClassNameString: `auto p = m.try_emplace(ClassNameString.str());`


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

https://reviews.llvm.org/D92674