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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
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.
----------------
arsenm wrote:
> This is unusably broken? You can't produce a valid G_* instruction with a cimm operand
`G_CONSTANT` uses a CImm, AFAIK
See `MachineIRBuilder::buildConstant` with `addCImm` uses.

Do you have example of instructions that need a direct imm? I can test them and fix them.

I'm also wondering if I should just go ahead and make immediates always emit G_CONSTANT except in specific cases (currently only G_CONSTANT I think?). It'd be a usability improvement.


================
Comment at: llvm/docs/GlobalISel/MIRPatterns.rst:109
+    (match (G_MUL $dst, $x, 1)),
+    (apply (COPY $dst, $x))
+  >;
----------------
arsenm wrote:
> maybe mention somewhere that you need to copy. 
> 
> How are patterns that discard their outputs handled? Does it error on those?
> How are patterns that discard their outputs handled? Does it error on those?

Good question, I think it will indeed error out.
Do you mean that there's patterns that just delete instructions? I could add a special case for `(apply)` to just erase the root. Would that work for you?


================
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.
----------------
arsenm wrote:
> Maybe should invest in having a variable length encoded match table. And/or could have a lookup table of known used wide constants
We can do that in the future, yes, but I think it's too much for now. Aren't most constants small anyway? Do we have patterns with constants >64 bits? In the meantime I'll add it to the limitations.
Also note that for a vector splat, this only takes the scalar element size, so if you want to check that a 4 x i64 has a splat value of 0, that works.


================
Comment at: llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/match-table-variadics.td:28
+  (defs root:$a),
+  (match (G_UNMERGE_VALUES $a, $b, $c, $d)),
+  (apply [{ APPLY }])>;
----------------
arsenm wrote:
> How would I go about enforcing element types here?
This is not yet possible, I forgot to add that but it's not much effort (in theory), all of the infrastructure is there :)
 
I'll add it. To be sure do you mean something like `(i32 $x)` to add a check for register type?



================
Comment at: llvm/utils/TableGen/GlobalISelMatchTable.h:1939-1940
   unsigned InsnID;
   int64_t Imm;
+  std::optional<LLTCodeGen> CImmLLT;
 
----------------
arsenm wrote:
> Could this be APInt? Should there be a separate APInt renderer?
We'd need a separate renderer APInt that can split the APInt into multiple 64-bit pieces & reassemble it after.
Do we even have a usecase for now for >64 bits here? I thought we didn't have any so I didn't think it was an issue.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156315