[GlobalISel] Allow matching & rewriting MIFlags

[Pierre-vh]

I am opening this ticket to gather feedback on how MIFlags matching/rewriting should be added to the MIR patterns used by the GISel combiners.

The feature itself is relatively simple to implement - I already got a WIP (https://reviews.llvm.org/D159111), but I am uncertain of the scope it should have, and of the syntax it should use.

Current Behavior

Currently, MIFlags handling in Combiners is non-existent, and we've inherited two things from the ISel machinery that make MIFlags handling non-obvious:

• Some flags are propagated to output instructions from the root instruction (see propagateFlags in GIMatchTableExecutorImpl.h
• We sometimes mutate opcodes instead of rebuilding output instructions. I assume we preserve flag in those cases, but we can't choose which instruction gets mutated so this is really not obvious.

My proposition would be to:

• Disable flag propagation completely for combiners. It's not really necessary, and I once even tried to make it work properly (so propagate even for instruction built using C++ code) and it actually made things worse/more confusing. I think combiners need to control flags better.
• Make GIM_MutateOpcode always clear MIFlags for combiners, to avoid accidental flag propagation.

Scope

I assume that at the bare minimum, we need to be able to match a given flag (and the absence of a flag), and write out some flags.
Do we also need a way to copy the flags from a source instruction? e.g. if we don't match all flags, but we want to preserve them.

Syntax

The syntax is a bit messy. MIR patterns are piggybacking on top of DAG patterns which is really not ideal (and I am actually starting to wonder if we wouldn't be better off just parsing chunks of MIR? But that's a discussion for another time).

In my WIP, the syntax looked like:

def TestNewInst : GICombineRule<
  (defs root:$dst),
  (match (G_ZEXT $dst, $src, MIFlags<[FmAfn, FmReassoc]>)),
  (apply (G_FADD $dst, $src, $src, MIFlags<[FmNoNans, FmAfn]>))>;

An alternative syntax i came up with is to use a dedicated DAG operator:

def TestNewInst : GICombineRule<
  (defs root:$dst),
  (match (G_ZEXT $dst, $src, (MIFlag FmReassoc, FmAfn)),
  (apply (G_FADD $dst, $src, $src, (MIFlag FmNoNans,, FmAfn))>;

The latter is more flexible, because we can somewhat neatly express a "not" operation

def TestNewInst : GICombineRule<
  (defs root:$dst),
  (match (G_ZEXT $dst, $src, (MIFlag FmReassoc, (not FmAfn))),  ; "not" is a DAG SDNode, but I guess it doesn't hurt to use it for this as well?
  (apply (G_FADD $dst, $src, $src, (MIFlag FmNoNans,, FmAfn))>;

My preference is on the DAG form. Thoughts?

[Pierre-vh]

cc @aemerson, @arsenm

[arsenm]

I also somewhat prefer the DAG form. I think the flag preservation needs to be explicit, but in the common case all flags are simply passed through. Maybe GICombineRule should have a preserve all property, otherwise you need one of these MIFlag operators

[Pierre-vh]

For preserving flags, I was thinking of something like this:

def TestNewInst : GICombineRule<
  (defs root:$dst),
  (match (G_ZEXT $dst, $src, (MIFlag FmReassoc, (not FmAfn))):$mi),
  (apply (G_FADD $dst, $src, $src, (MIFlag $mi, (not FmReassoc))))>;

The idea is to just allow passing a matched instruction name in the MIFlags operator, and it'd copy the flags from that instruction. The (not FmReassoc) just clears that flag afterwards.

So the behavior would be:

• No flags on output instructions by default
• Preserving flags is opt-in, by using (MIFlags $mi) on the output instruction

I think this is a good compromise. Would that work for you?

[Pierre-vh]

@arsenm I've been working on this a bit, but I've been wondering: do we expect many patterns to need MIFlags handling?
This can be implemented just fine, but it'll be a bit of effort (say a day of work + review). I'm wondering if the effort is worth it at all currently.

If only a few rules need MIFlags handling, they can easily do it in C++ and I'd rather spend my time working on things that will help port more rules to MIR patterns. I'd still do this, but as a "QoL" update later.

Also for the flag preservation part, do you think we need that at all, or will the majority of pattern be fine with just checking for a few flags, and writing flags out without needing to copy flags over from an input inst?

[arsenm]

@arsenm I've been working on this a bit, but I've been wondering: do we expect many patterns to need MIFlags handling?

I think a decent number of combines do inspect these. It's not the majority, but there are quite a few checks on the flags in DAGCombiner.

Also for the flag preservation part, do you think we need that at all, or will the majority of pattern be fine with just checking for a few flags, and writing flags out without needing to copy flags over from an input inst?

I think just dropping all flags would be problematic. Basic canonicalizing transforms (e.g. commute) losing flags would break later more meaningful transforms