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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added a comment.

In D153757#4478556 <https://reviews.llvm.org/D153757#4478556>, @aemerson wrote:

> 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.

Definitely agree, I've tried to add as much docs as I could think of without making it too verbose, please let me know if you think it still needs more.

I also thought about splitting this up in different files to make it more readable but I feel like the logic is so connected everywhere that it'd have the opposite effect - I'd probably end up making things more confusing/verbose by trying to over-split things. Maybe the Patterns hierarchy could go in a separate file but that's about it. It's only 200 or so lines saved.

> 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?

`SpecificBumpPtrAllocator` is an interesting idea, if we'd prefer that I can add it. Though in this case ownership semantics are simple: every `Pattern` is owned by the `CombineRuleBuilder`'s StringMaps, `unique_ptr` is just used to transfer ownership and keep pointers stable. Once a pattern is moved into one of the maps, it never moves again.
I like it because it's a way to express that a pattern isn't part of the builder yet. I'll make that clearer in the documentation but please let me know if that's not enough, then I can use the `SpecificBumpPtrAllocator`.

I've also moved some definitions out of line to improve readability.



================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:149
+  bool needsUnreachable() const {
+    return !StringRef(Code).starts_with("return");
+  }
----------------
aemerson wrote:
> Should this strip leading whitespace?
This object is only created through `CXXPattern`, which already trims all leading/trailing whitespace in the constructor. I'll document it


================
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];
+
----------------
aemerson wrote:
> Use structured binding to unpack?
It's a struct so I'd rather avoid it in this case, it could lead to subtle issues if fields are reordered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153757