[PATCH] D153757: [RFC][TableGen][GlobalISel] Add Combiner Match Table Backend

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

One problem I had with the existing combiner was that it was difficult to understand for someone unfamiliar with it. I think it would be helpful to have some more documentation in some places, for example some of the methods in `CombineRuleBuilder`. If we can make this code more accessible it will lower the friction for new contributors to make improvements and fixes.

Another useful piece of docs is a comment somewhere describing the ownership semantics. I see unique_ptr allocations and ownership transfers but it's not clear looking at the code why some pointer is being moved. On that topic, just freestyling here: would it be possible to use `SpecificBumpPtrAllocator` so we can just destroy all the objects at once later and remove the need for keeping track of ownership?



================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:149
+  bool needsUnreachable() const {
+    return !StringRef(Code).starts_with("return");
+  }
----------------
Should this strip leading whitespace?


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:265
+private:
+  SmallVector<const CodeGenInstruction *, 2> Insts;
+};
----------------
Can bump this to 4 since we often have >2 opcodes to wip_match_opcode


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:671
+      // Create an entry, no matter if it's a use or a def.
+      auto &Entry = OperandTable[Operand.Name];
+
----------------
Use structured binding to unpack?


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:674-681
+      if (Operand.IsDef) {
+        if (Entry.MatchPat) {
+          PrintError("Operand '" + Operand.Name +
+                     "' is defined multiple times in the 'match' patterns");
+          return false;
+        }
+        Entry.MatchPat = IP;
----------------
Invert condition to early-exit if `!Operand.IsDef`


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:691
+      // Create an entry, no matter if it's a use or a def.
+      auto &Entry = OperandTable[Operand.Name];
+
----------------
ditto


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:694
+      // Record defs.
+      if (Operand.IsDef) {
+        if (!Entry.MatchPat) {
----------------
ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153757