[PATCH] D141135: [RFC][GlobalISel] Replace the current GlobalISel matcher with a bottom-up matcher

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

>From my reading of this review, it seems no one has a fundamental disagreement with the overall direction. In that case, I think it's ready to go into a conventional implementation review.



================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:89
+  (a)
+  If a dimension of the lookup table is 1 then that dimenion, the corresponding
+  row of the compression table, and the corresponding operand can be omitted.
----------------
dimension


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:862
+    SmallVector<SmallVector<BitVector, 32>, 3>
+        ChaseToHOD; // Mapping from the Chase set encoding to a HOD match set.
+    GIMatchTreeAutomaton::Table T;
----------------
Please document more what you mean by Chase and HOD sets and why this mapping is needed. It wasn't obvious to me that HOD here meant "Hoffman and O'Donnell". 


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:869-870
+    T.C = OwningArrayRef<unsigned>(T.DimC * DimMatchSets);
+    for (unsigned I = 0, E = T.C.size(); I < E; ++I)
+      T.C[I] = 0;
+
----------------
`std::fill()`?


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:112
+  (c)
+  The implementation does not care about memory management. A context object
+  holding a bump allocator could be introduced to free most of the memory after
----------------
Pierre-vh wrote:
> A bit scary to read and it would be useful to elaborate. Does it leak? Is it inefficient in terms of space (allocates too much) or time (e.g. chains of owned ptrs need to be freed and it takes time) ?
Yes, if this leaks (even harmlessly in the case of llvm-tblgen) it could break bots running valgrind etc.


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

https://reviews.llvm.org/D141135