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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Combiner.cpp:125
+    for (MachineInstr *MI : ChangedInstrs) {
+      LLVM_DEBUG(dbgs() << "Adding path for: "; MI->print(dbgs()));
+      add(MI);
----------------
can just directly print *MI


================
Comment at: llvm/lib/CodeGen/GlobalISel/Combiner.cpp:168-169
     GISelWorkList<512> WorkList;
-    WorkListMaintainer Observer(WorkList);
+    SmallVector<MachineInstr *, 32> ChangedInstrs;
+    WorkListMaintainer Observer(WorkList, ChangedInstrs);
     GISelObserverWrapper WrapperObserver(&Observer);
----------------
Should move construction of these out of the loop too?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64O0PreLegalizerCombiner.cpp:75
+bool AArch64O0PreLegalizerCombinerInfo::combine(
+    DenseMap<MachineInstr *, unsigned> &MatchSets,
+    GISelChangeObserver &Observer, MachineInstr &MI,
----------------
Should use a typedef/using for the set type


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:70
+
+#include "../CodeGenInstruction.h"
+
----------------
Avoid relative include paths


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:170
+public:
+  EnumSetRangeImpl(const ArrayRef<SetT> &Dimension, bool IsAtEnd = false)
+      : Dimension(Dimension), AtEnd(IsAtEnd || !Dimension.size()) {
----------------
ArrayRefs should be passed by value


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:353
+                       const ArrayRef<unsigned> Data, StringRef Paren) {
+  assert(Dim.size() > 0 && "Expected at least one dimension");
+  assert(Paren.size() == 2 && "Expected pair of parenthesis");
----------------
!empty()


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:362
+    if (Idx % LastDim)
+      OS << ",";
+    else {
----------------
Single quotes here and elsewhere for single character printing


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:672-678
+  auto getRoot = [&RootIdx, &MatchDag]() -> const GIMatchDagInstr * {
+    for (const auto &Root : enumerate(MatchDag.roots())) {
+      if (Root.index() == RootIdx)
+        return Root.value();
+    }
+    return nullptr;
+  };
----------------
Can this just be a standalone function if you're passing in the two items explicitly anyway?


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:751
+
+  // If DAAGRoot is an instruction, then traverse all edges originating there.
+  if (getOpcodePredicate(DAGRoot)) {
----------------
Typo DAAGRoot


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:808
+void PatternForestBuilder::dumpMS(T Sets, StringRef Name, unsigned No) const {
+  llvm::dbgs() << Name << "_" << No << " =\n";
+  for (auto &MS : Sets) {
----------------
Don't need llvm::


================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:978
+      // Opnds[] is the Hoffmann/O'Donnel match set.
+      SmallVector<BitVector, 3> Opnds;
+      Opnds.resize_for_overwrite(T.DimC);
----------------
Hoist construction out of loop?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141135