[PATCH] D156315: [RFC][GlobalISel] Overhauled MIR Patterns Support for Combiners

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/docs/GlobalISel/MIRPatterns.rst:189-192
+  def Foo : GICombineRule<
+    (defs root:$dst),
+    (match (G_FOO $tmp, $src), (G_BAR $dst, $tmp)),
+    (apply (G_IMPLICIT_DEF $dst))>;
----------------
It doesn't have to be in the initial patch, but can this be addressed later? If we migrate to using MIR patterns more often, this seems wasteful (generating a new inst, observer call, CSE, DCE)

Similar thing for the replacing a register case, it would be good to have a way to trigger a `MRI.replaceRegWith()`.


================
Comment at: llvm/docs/GlobalISel/MIRPatterns.rst:348
+
+  // This fragment does not bind $x to an operand in any
+  // all of its alternative patterns.
----------------
s/in any all/in all


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:677-678
+
+      if (!Operand.isDef())
+        continue;
+
----------------
can move this above the definition of `Def`


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:1022
+
+bool PatFrag::checkSemantics() {
+  for (const auto &A : Alts) {
----------------
I'd prefer to not use single letter variable names, `Alt` and `Op` are fine for me.


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:1234
+
+  bool mapInputCodeExpansions(const CodeExpansions &ParentCEs,
+                              CodeExpansions &PatFragCEs,
----------------
This method could use some documentation either here or in the definition,


================
Comment at: llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp:3371-3379
 
+  std::vector<LLTCodeGen> TypeObjects;
+  append_range(TypeObjects, KnownTypes);
+  llvm::sort(TypeObjects);
+
+  // Hack: Avoid empty declarator.
+  if (TypeObjects.empty())
----------------
Should we use a SmallVector for these?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156315