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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D141135#4031707 <https://reviews.llvm.org/D141135#4031707>, @Kai wrote:

> Therefore I store the state in the `Combiner`, and pass it down to `CombinerInfo::combine`. An alternative would be to add a new member to `CombinerInfo`. This is just for the life time requirement, the usage is only in the `tryCombineAll()`, but I have not yet found a better solution.

I think putting in CombinerInfo would be better



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerInfo.h:70
   /// not automatic.
-  virtual bool combine(GISelChangeObserver &Observer, MachineInstr &MI,
+  virtual bool combine(DenseMap<MachineInstr *, unsigned> &MatchSets,
+                       GISelChangeObserver &Observer, MachineInstr &MI,
----------------
I think this need to be a using/typedef 


================
Comment at: llvm/lib/CodeGen/GlobalISel/Combiner.cpp:106
+    while (!WorkStack.empty()) {
+      auto &MIFlagPair = WorkStack.back();
+      MachineInstr *MI = MIFlagPair.first;
----------------



================
Comment at: llvm/utils/TableGen/GICombinerEmitter.cpp:59
+    "gicombiner-compact-yaml",
+    cl::desc("Use compact representation of tables in YAML"),
+    cl::cat(GICombinerEmitterCat));
----------------
I'd assume compact would be the default?


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:117
+
+namespace llvm {
+
----------------
Don't need  llvm namespace?


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:337
+    if (Idx % LastDim)
+      OS << ",";
+    else {
----------------
single quotes


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:358
+    if ((Idx + 1) % LastDim == 0) {
+      OS << " " << PC;
+      for (unsigned PrevFac = LastDim, I = 0, E = Dim.size() - 1; I < E; ++I) {
----------------
Single quotes 


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:366
+        Indent -= 2;
+        OS << "\n";
+        OS.indent(Indent) << PC;
----------------
Single quotes


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:818
+    for (auto &L : Labels) {
+      assert(L.SA.size() == L.getNumUseOpnds() && "Size missmatch");
+      for (auto PatternSet : EnumSet<SetOfSetOfPatterns, BitVector>(L.SA)) {
----------------
Typo missmatch


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.h:29
+
+#include "../CodeGenTarget.h" // For enumerating instructions.
+
----------------
Should still avoid relative include paths


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

https://reviews.llvm.org/D141135