[PATCH] D159111: [TableGen][GlobalISel] Add support for matching/writing MIFlags in MIR Patterns

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/docs/GlobalISel/MIRPatterns.rst:171
+mentionned multiple times, it's only checked/added once.
 
 Limitations
----------------
Pierre-vh wrote:
> Pierre-vh wrote:
> > arsenm wrote:
> > > Pierre-vh wrote:
> > > > arsenm wrote:
> > > > > Should mention the default behavior. Flags should be dropped unless explicitly preserved. Can you and/or the incoming flags?
> > > > The current behavior is that the output instruction is a "blank canvas" so no flags are preserved, and no there's no way to fetch the flags from an instruction & preserve them (hence why I added the `PropagateFlags` bool).  
> > > > 
> > > > Is there any use case where we need to match only one flag and optionally preserve the other flags?
> > > > 
> > > > If the set of possibilities is relatively small, a PatFrag can do it - just explicitly match & rewrite them. Not ideal but it works. I'm sure it could all be written with some loop in TableGen as well.
> > > > 
> > > > Otherwise, maybe we could have a builtin for that, something like `GIMIFlagsAnd<$a, $b>` but we also need to be careful about `GIR_MutateOpcode` (which may erase the flags from the original inst), so it's not 100% trivial.
> > > > 
> > > Most cases involving flags are match one or two and preserve all. There are a fair number of match one or two and drop specific flag
> > So what we need is:
> > - A way to copy flags from a matched instruction to a new instruction
> >   - Is the old PropagateFlags system good enough? It only copies the flags from the root AFAIK. I'm also wondering if it's not better to make flags not preserved by default, and add something like `GICopyMIFlags<$mi>` to explicitly copy them)
> > - A way to remove a flag (e.g. `GIRemoveMIFlag`)
> For the PropagateFlag system, my worry is something like this: Imagine a combine where you want to transform (fneg (fmul)) into (fmul (fneg)).
> 
> With the current system, the G_FNEG would be the root and its flags would be propagated on the G_FMUL, but if what we actually want is to preserve the matched G_FMUL flag on the new G_FMUL, it doesn't work as expected.
> 
> It's why I assume it's better to have to explicitly import/set flags in combine rules. I think it's less confusing.
> 
> Though it can get verbose quickly if you want to add a few flags and unset one, e.g.
> ```
> (G_FMUL $a, $b, $c, MIFlagsCopy<"$matchedfmul">, MIFlagsSet<[a, b]>, MIFlagsUnset<c>)
> ```
> Maybe we enforce a single `MIFlags` operand but give it more parameters, so we can write `MIFlags<"$matchedfmul", [a, b], [c]>` ?
I have no real idea what the "PropagateFlag" was, but it certainly can't be a global mode. It has to be explicit in the pattern. I think we need and/or and unset


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159111