[PATCH] D110383: [JITLink][NFC] Add TableManager to replace PerGraph...Builder pass

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:212-215
+  GOTTableManager_ELF_x86_64 GOT;
+  PLTTableManager_ELF_x86_64 PLT(GOT);
+  TLSInfoTableManager_ELF_x86_64 TLSInfo;
+  runFixersOnGraph(G, GOT, PLT, TLSInfo);
----------------
I really like how this pattern worked out. Very neat!


================
Comment at: llvm/lib/ExecutionEngine/JITLink/TableManager.h:24-44
+static void tryEdgeFixer(LinkGraph &G, Block *B, Edge &E) {}
+
+template <typename FixerT, typename... FixerTs>
+static void tryEdgeFixer(LinkGraph &G, Block *B, Edge &E, FixerT &&Fixer,
+                         FixerTs &&...Fixers) {
+  if (!Fixer.fixEdge(G, B, E))
+    tryEdgeFixer(G, B, E, std::forward<FixerTs>(Fixers)...);
----------------
It's just occurred to me that this is a kind of visitor pattern.

I think it makes sense to move these functions into their JITLink.h and rename them to `visitEdge` (from `tryEdgeFixer`) and `visitExistingEdges` (from `runFixersOnGraph`). The `fixEdge` method should be renamed to `visitEdge` and we should document that `visitEdge` should return 'true' (if the edge should be considered "dealt with", or 'false' (if we want the edge to also be acted on by any subsequent visitors).

These changes could be made either before you land the patch, or afterwards in-tree. Whichever you prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110383