This is an archive of the discontinued LLVM Phabricator instance.

[TableGen][GlobalISel] Add support for matching/writing MIFlags in MIR Patterns
AbandonedPublic

Authored by Pierre-vh on Aug 29 2023, 8:27 AM.

Details

Summary

Allow matching one or more MIFlags, and writing them out as well.

Depends on D158975

Diff Detail

Event Timeline

Pierre-vh created this revision.Aug 29 2023, 8:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2023, 8:27 AM
Pierre-vh requested review of this revision.Aug 29 2023, 8:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2023, 8:27 AM
arsenm added inline comments.Aug 29 2023, 2:57 PM
llvm/docs/GlobalISel/MIRPatterns.rst
170
173

Should mention the default behavior. Flags should be dropped unless explicitly preserved. Can you and/or the incoming flags?

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
566–568

Don't see why this needs to be configurable

Pierre-vh marked 2 inline comments as done.Aug 29 2023, 11:28 PM
Pierre-vh added inline comments.
llvm/docs/GlobalISel/MIRPatterns.rst
173

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.

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
566–568

I was wondering what happens if a root has some MI flags, and the pattern rewrites the root & other instructions. Do we really have to propagate flags from the root inst in the combiner?

I have no strong opinion, it just seemed wrong to me so I disabled it for the combiner.

Pierre-vh marked an inline comment as done.

Update docs

arsenm added inline comments.Sep 5 2023, 8:24 AM
llvm/docs/GlobalISel/MIRPatterns.rst
173

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

Pierre-vh added inline comments.Sep 6 2023, 12:10 AM
llvm/docs/GlobalISel/MIRPatterns.rst
173

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)
Pierre-vh added inline comments.Sep 7 2023, 5:27 AM
llvm/docs/GlobalISel/MIRPatterns.rst
173

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]> ?

arsenm added inline comments.Sep 7 2023, 9:31 AM
llvm/docs/GlobalISel/MIRPatterns.rst
173

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