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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

This is much clearer than what was documented before



================
Comment at: llvm/docs/GlobalISel/MIRPatterns.rst:87-89
+* Immediates always emit a CImm in the ``apply`` pattern of a
+  ``GICombineRule``. There is no way to implicitly emit a ``G_CONSTANT``,
+  or emit an simple immediate operand.
----------------
This is unusably broken? You can't produce a valid G_* instruction with a cimm operand


================
Comment at: llvm/docs/GlobalISel/MIRPatterns.rst:109
+    (match (G_MUL $dst, $x, 1)),
+    (apply (COPY $dst, $x))
+  >;
----------------
maybe mention somewhere that you need to copy. 

How are patterns that discard their outputs handled? Does it error on those?


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h:730
         LLT Ty = MRI.getType(MO.getReg());
-        Value = SignExtend64(Value, Ty.getSizeInBits());
+        // If the type is > 64 bits, it can't be a constant int, so we bail
+        // early because SignExtend64 will assert otherwise.
----------------
Maybe should invest in having a variable length encoded match table. And/or could have a lookup table of known used wide constants


================
Comment at: llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/patfrag-errors.td:221
+  ]>;
+// CHECK: :[[@LINE+2]]:{{[0-9]+}}: error: expected operand 1 of 'ExpectedMO' to be a MachineOperand
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Failed to parse pattern: '(ExpectedMO ?:$dst, (i32 0))'
----------------
Could this also state what it actually was instead?


================
Comment at: llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-errors.td:4
+// RUN: FileCheck %s -implicit-check-not=error:
+
+include "llvm/Target/Target.td"
----------------
Lots of diagnostic tests is very good


================
Comment at: llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-errors.td:172
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Failed to parse pattern: '(G_BUILD_VECTOR ?:$x)'
+def too_little_ops_for_variadic : GICombineRule<
+  (defs root:$x),
----------------
too little/too few


================
Comment at: llvm/utils/TableGen/GlobalISelMatchTable.h:1939-1940
   unsigned InsnID;
   int64_t Imm;
+  std::optional<LLTCodeGen> CImmLLT;
 
----------------
Could this be APInt? Should there be a separate APInt renderer?


================
Comment at: llvm/utils/TableGen/GlobalISelMatchTable.h:1958
+      assert(Table.isCombiner() &&
+             "ConstantINt immediate are only for combiners!");
+      Table << MatchTable::Opcode("GIR_AddCImm")
----------------
Typo ConstantINt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156315