[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:
> Pierre-vh wrote:
> > 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.
> addImm uses correspond to immarg arguments. Everything else uses G_CONSTANT with a isCImm argument as a materialized constant
Right, so I can just look at uses of `addImm` and special-case all of those instructions, then default to inserting a G_CONSTANT for the rest? Does that work?

Should I just special-case them in code, or add some field to GenericInstruction in TableGen and infer it from there?


================
Comment at: llvm/docs/GlobalISel/MIRPatterns.rst:109
+    (match (G_MUL $dst, $x, 1)),
+    (apply (COPY $dst, $x))
+  >;
----------------
arsenm wrote:
> Pierre-vh wrote:
> > 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?
> We have some special case atomic optimizations that discard the result if use_empty, it's currently dealt with in selection but theoretically could be a combine
If I understand correctly it'd look like this?
```
(match (G_ATOMIC_FOO $x, $y), "return MRI.use_empty(${x}.getReg())"))
(apply) // somehow delete it
```

Then yeah, that's not supported so far. I can add a special case to just delete the root if there are no apply patterns if you think that'd work?


An easy workaround would be to just set $x to IMPLICIT_DEF, but if G_ATOMIC_FOO has no defs then there's no workaround other than using C++ in the (apply).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156315