[VP][DAGCombiner] Introduce generalized pattern match for vp sdnodes.

[fakepaper56]

Summary

The patch tries to solve duplicated combine work for vp sdnodes. The idea is to
introduce MatchConext that verifies specific patterns and generate specific node
infromation. There is two MatchConext in DAGCombiner. EmptyMatcher is for
normal nodes and VPMatcher is for vp nodes.

The idea of this patch is come form Simon Moll's proposal [0]. I only fixed some
minor issues and added few new features in this patch.

[0]: https://github.com/sx-aurora-dev/llvm-project/commit/c38a14484aa2945f3b05369560b65916dd832f76

[fakepaper56]

@simoll There is some difference between my implement and yours:

1. Operand with true mask does not fail in match() of VPMatchContext.
2. Operand with normal opcode does not fail in match() of VPMatchContext if its opcode is same as the input opcode.
3. Make VPMatchContext be only constructed by vp root nodes. It could help for point 2.
4. I removed the member function getFunctionOpCode and isCompatible.

Do you have any idea for my change?

[craig.topper]

llvm/include/llvm/CodeGen/ISDOpcodes.h

1350

OpCode->Opcode to match the usual capitalization in SelectionDAG.

1354

OpCode -> Opcode

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

858

This is overindented

866

We don't usually use Optional for Flags. We just default it to SDNodeFlags()

877

Drop else after return

[fakepaper56]

Address Craig's comment.

[fakepaper56]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

866

I think using Optional is to represent both DAG.getCode() w/ SDNodeFlags and DAG.getCode() w/o SDNodeFlags.

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

866

The SelectionDAG::getNode functions being called always take an SDNodeFlags they just default construct one if it isn't passed explicitly. Why can't we do the same in the context?

[fakepaper56]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

866

I think SelectionDAG::getNode uses FlagInserter when not having SDNodeFlags argument. The below code is the reason why I think so,

SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
                              SDValue Operand) {
  SDNodeFlags Flags;
  if (Inserter)
    Flags = Inserter->getFlags();
  return getNode(Opcode, DL, VT, Operand, Flags);
}

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

866

Ok. I forgot about flag inserter. We used to default construct flags. I have another idea I'm going to try

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

953

MaskPos and VLenPos are unused in release builds and will give a warning.

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

867

Optional no longer exists. It was replaced with std::optional

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

862

For empty context, I think we can have a single getNode like this

template <typename... ArgT>
SDValue getNode(ArgT &&... Args) {
  return DAG.getNode(std::forward<ArgT>(Args)...);
 }

Not sure if there's something similar we can do for the other VPMatchContext.

[fakepaper56]

Address Craig's comment.

[fakepaper56]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

862

Thank you for providing the way.

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

14909

N0 vs N1 ?

[frasercrmck]

llvm/include/llvm/CodeGen/ISDOpcodes.h

1350

I'm not mad about the term "function opcode" - it doesn't sound right to me. I don't have a better suggestion right now though. I'm just commenting this in case other people feel similarly.

llvm/test/CodeGen/RISCV/rvv/fold-vp-fadd-and-vp-fmul.ll

2

Pre-committing this test would be nice, as it'd allow us to better see the changes this patch allows us.

[fakepaper56]

Rebase and fix typos.

[fakepaper56]

llvm/test/CodeGen/RISCV/rvv/fold-vp-fadd-and-vp-fmul.ll

2

Thank your idea. I create a new revision D142351 for that.

[spatel]

llvm/include/llvm/CodeGen/ISDOpcodes.h

1350

Just a passing comment because it took me 3 tries to translate that "mad" from the American English "angry" to the British English "crazy" and then to "happy". :)

Possible alternate:

getNonPredOpcodeForVP()

or less contracted:

getNonPredicatedOpcodeForVPOpcode()

or:

getBaseOpcodeForVPOpcode()

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

917

Unless I can figure out some template magic, I think I'd like to see each of these getNode's split into two functions. One that takes SDNodeFlags and one that doesn't. As it as now we're kind of abusing std::optional to count parameters.

[fakepaper56]

Split getNode of VPMatchContext to some with SDNodeFlags and others with SDNodeFlags.

[fakepaper56]

llvm/include/llvm/CodeGen/ISDOpcodes.h

1350

I prefer getBaseOpcodeForVPOpcode since base opcodes also have no effective vector length operands. Does anyone have another idea?

[fakepaper56]

Rename getFunctionOpcodeForVP to getBaseOpcodeForVP.

[craig.topper]

llvm/include/llvm/CodeGen/ISDOpcodes.h

1353

Should this be renamed too?

llvm/include/llvm/IR/VPIntrinsics.def

111

And this?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

892

Rename FuncOpc to match new name

14809

I think we can use std::is_same_v<MatchContextClass, VPMatchContext> since we are on C++17

[fakepaper56]

llvm/include/llvm/IR/VPIntrinsics.def

111

VP_PROPERTY_FUNCTIONAL_INTRINSIC also uses "FUNCTIONAL", so I am preferred to use my original naming here for consistency now.

[fakepaper56]

Use is_same_v and rename getVPForFunctionOpcode to getVPForBaseOpcode.

[craig.topper]

LGTM

[mehdi_amini]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

870

This seems unused? At least I'm getting a build warning now.

[DamonFool]

Build failure after this.
Please see https://reviews.llvm.org/D143551 .
Thanks.