[PATCH] D93654: [TableGen] Change getAllDerivedDefinitions() to return an ArrayRef

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

General question: What's the motivation for this work series/direction? Did the lack of caching show up in profiles? Has the addition of profiling shown to be a significant improvement in runtime?
But yeah, if the caching makes sense - seems nice to do this tidy up work that avoids making copies that are no longer needed.



================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:3134
 void CodeGenDAGPatterns::ParseComplexPatterns() {
-  std::vector<Record*> AMs = Records.getAllDerivedDefinitions("ComplexPattern");
+  std::vector<Record*> AMs = // Copy the returned vector.
+      Records.getAllDerivedDefinitions("ComplexPattern");
----------------
I'd probably not put the comment here - it splits the expression possibly unnecessarily (I guess generally it doesn't fit on one 80 col line anyway? But still I'd avoid it - would make refactoring the expression more fussy, etc). Could you put it on a separate line before this statement? (& maybe rephrase it to explain the need to copy - which will be easier with the full 80 columns for the comment, I suspect - like "Make a copy of the forms due to subsequent modifications"?)

Also, it looks like in most of these instances, a copy isn't actually needed - maybe follow-up commits could remove these copies in favor of non-copying algorithms. For instance in this function, the elements are popped back from the vector, but equivalent non-mutating code might look something like:
```
for (Record* R : llvm::reverse(AMs))
  ComplexPatterns.insert(std::make_pair(R, R));
```

(Might be easier to make those changes to non-mutation first/in an independent commit, then that'd simplify this change by no longer needing the copies and comments)

At least so far as I can tell, that's all this code is doing - and the other copy cases in this file could be similarly modified. The only copy that looks a bit more necessary seems to be "getAllOpInterfaceDefinitions"


================
Comment at: mlir/tools/mlir-tblgen/OpInterfacesGen.cpp:81
+
+  // TODO: Do we really need to copy the defs vector?
+
----------------
Looks like this is necessary because one caller passes the result of `getAllOpInterfaceDefinitions` to this ctor - so it needs storage. A type erased filtering iterator, or perhaps not... if InterfaceGenerator is just an implementation convenience base class - it could be a template, perhaps a CRTP base that avoids the need for getAllOpInterfaceDefinitions to manifest the copy in memory by providing an iteration API rather than a data structure. But maybe that involves too much stuff having to move into a header? Not sure.


================
Comment at: mlir/tools/mlir-tblgen/OpInterfacesGen.cpp:83-84
+
+  InterfaceGenerator(const std::vector<llvm::Record *> &defs, raw_ostream &os)
+      : defs(defs), os(os) {} // Copy the defs vector.
 
----------------
This change looks incorrect to me - what was the motivation to change the parameter from rvalue ref? 
(though it might, regardless of this change, be better written as pass-by-value - then callers with temporaries (like those passing the result of getAllOpInterfaceDefinitions) or explicit std::move for the parameter would be benefit from being able to move into the InterfaceGenerator - and callers that want to keep their own copy of the vector could do so as well - https://abseil.io/tips/117 discusses the general idea in more detail)


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

https://reviews.llvm.org/D93654