This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by fakepaper56 on Jan 16 2023, 9:56 PM.

Details

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

Diff Detail

Event Timeline

fakepaper56 created this revision.Jan 16 2023, 9:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 9:56 PM
fakepaper56 requested review of this revision.Jan 16 2023, 9:56 PM

@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 retitled this revision from [VP][DAGCombiner] Introduce gneralized pattern match for vp sdnodes. to [VP][DAGCombiner] Introduce generalized pattern match for vp sdnodes..Jan 17 2023, 9:12 AM
craig.topper added a reviewer: reames.
craig.topper added inline comments.Jan 17 2023, 9:19 AM
llvm/include/llvm/CodeGen/ISDOpcodes.h
1348

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

1352

OpCode -> Opcode

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
846

This is overindented

854

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

865

Drop else after return

Address Craig's comment.

fakepaper56 marked 4 inline comments as done.Jan 17 2023, 10:15 PM
fakepaper56 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
854

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

craig.topper added inline comments.Jan 17 2023, 11:31 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
854

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 added inline comments.Jan 18 2023, 1:50 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
854

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 added inline comments.Jan 18 2023, 9:03 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
854

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

craig.topper added inline comments.Jan 18 2023, 9:07 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
941

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

craig.topper added inline comments.Jan 18 2023, 9:14 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
855

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

craig.topper added inline comments.Jan 18 2023, 9:20 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
850

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.

Address Craig's comment.

fakepaper56 marked 3 inline comments as done.Jan 20 2023, 12:12 AM
fakepaper56 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
850

Thank you for providing the way.

RKSimon added inline comments.Jan 20 2023, 7:16 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14715

N0 vs N1 ?

frasercrmck added inline comments.Jan 23 2023, 1:53 AM
llvm/include/llvm/CodeGen/ISDOpcodes.h
1348

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 marked an inline comment as done.

Rebase and fix typos.

fakepaper56 marked 2 inline comments as done.Jan 23 2023, 5:34 AM
fakepaper56 added inline comments.
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 added inline comments.Jan 23 2023, 6:17 AM
llvm/include/llvm/CodeGen/ISDOpcodes.h
1348

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 added inline comments.Jan 23 2023, 1:54 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
905

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 marked an inline comment as done.

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

fakepaper56 marked an inline comment as done.Jan 28 2023, 7:16 AM
fakepaper56 added inline comments.
llvm/include/llvm/CodeGen/ISDOpcodes.h
1348

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

Rename getFunctionOpcodeForVP to getBaseOpcodeForVP.

craig.topper added inline comments.Feb 1 2023, 10:35 PM
llvm/include/llvm/CodeGen/ISDOpcodes.h
1351

Should this be renamed too?

llvm/include/llvm/IR/VPIntrinsics.def
111

And this?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
880

Rename FuncOpc to match new name

14615

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

fakepaper56 added inline comments.Feb 2 2023, 1:25 AM
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.

Use is_same_v and rename getVPForFunctionOpcode to getVPForBaseOpcode.

fakepaper56 marked 2 inline comments as done.Feb 2 2023, 1:37 AM
This revision is now accepted and ready to land.Feb 7 2023, 4:33 PM
This revision was landed with ongoing or failed builds.Feb 7 2023, 9:45 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
858

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

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