[SelectionDAG] Introducing the SelectionDAG pattern matching framework #78654

mshockwave · 2024-01-19T00:46:30Z

Akin to llvm::PatternMatch and llvm::MIPatternMatch, the llvm::SDPatternMatch introduced in this patch provides a DSL-alike framework to match SDValue / SDNode with a more succinct syntax.

TBA...

mshockwave · 2024-01-19T00:50:49Z

Note: I only added the most basic and hopefully the most common patterns in this patch. Feel free to propose other patterns that you thought are important.

topperc · 2024-01-19T01:00:32Z

llvm/include/llvm/CodeGen/SDPatternMatch.h

+  explicit NUses_match(const Pattern &P) : P(P) {}
+
+  bool match(const SelectionDAG *DAG, SDValue N) {
+    return N && N->hasNUsesOfValue(NumUses, N.getResNo()) && P.match(DAG, N);


hasNUsesOfValue can be expensive on nodes with multiple results. It can't return true until it has checked all uses. Uses for all results are stored in the same list.

We try to always check opcode first before checking number of uses. It's better to ask if an opcode is an ISD::ADD first since it only has 1 result. Than to ask for the number of uses without knowing what type of node it is.

We try to always check opcode first before checking number of uses. It's better to ask if an opcode is an ISD::ADD first since it only has 1 result. Than to ask for the number of uses without knowing what type of node it is.

I think a simple solution could be swapping hasNUsesOfValue with the succeeding P.match call, as we have to fulfills these two conditions anyway:

P.match(DAG, N) && N->hasNUsesOfValue(NumUses, N.getResNo())

As SDNode::hasNUsesOfValue is pretty expensive

RKSimon · 2024-01-31T11:40:21Z

Random question - will this play nice with the template approach taken for VP node (e.g. #80105)?

RKSimon · 2024-02-11T15:53:39Z

@mshockwave reverse-ping

mshockwave · 2024-02-12T18:32:08Z

Random question - will this play nice with the template approach taken for VP node (e.g. #80105)?

My apologies for the delay.
I think supporting VPMatchContext::match in SDPatternMatch makes a lot of sense and I'll add that. Though I'm not sure if we should bring VPMatchContext::getNode into SDPatternMatch as well.

mshockwave · 2024-02-12T23:53:45Z

I'd just introduced the concept of match context to SDPatternMatch: now a user can pattern match a SDValue under certain match context using sd_context_match. A match context is really similar to that in DAGCombiner (e.g. VPMatchContext) except it only needs to implement getDAG() and getTLI(); The match(SDValue, unsigned) is optional.
Furthermore, you can use custom contexts only in a sub-pattern via m_Context(<custom context>, <sub pattern>).

Switching to this match context design also solves one of the problem where previously, the user had to provide a SelectionDAG in order to use any TLI hook, which was inconvenient in places lack of SelectionDAG. Now we can independently provide TLI instances to sd_context_match.

RKSimon

Please can you add m_Deferred handling?

llvm/include/llvm/CodeGen/SDPatternMatch.h

nikic · 2024-02-13T16:16:52Z

llvm/include/llvm/CodeGen/SDPatternMatch.h

+  // Optional trait function(s)
+
+  /// Return true if N effectively has opcode Opcode.
+  // bool match(SDValue N, unsigned Opcode)


Why make this optional instead of having a default implementation of N->getOpcode() == Opcode?

Users won't necessary inherit BasicContext here to create a new match context. The default implementation for match is implemented in places like Opcode_match::match, toggled by ctx_has_match.

Do we expect there to be more contexts than BaseContext and VPMatchContext? I don't think that custom context will be common enough that we have to optimize for implementing one method less.

Do we expect there to be more contexts than BaseContext and VPMatchContext? I don't think that custom context will be common enough that we have to optimize for implementing one method less.

I'm convinced, I'll change this in the next update.

It's fixed now.

llvm/include/llvm/CodeGen/SDPatternMatch.h

mshockwave · 2024-02-15T00:54:16Z

Please can you add m_Deferred handling?

~~Of course. I'll add it in the next update.~~
@RKSimon It's done now.

llvm/include/llvm/CodeGen/SDPatternMatch.h

The major one was simplifying Node_match into Operands_match.

nikic · 2024-02-22T14:23:41Z

llvm/include/llvm/CodeGen/SDPatternMatch.h

+  explicit Opcode_match(unsigned Opc) : Opcode(Opc) {}
+
+  template <typename MatchContext>
+  std::enable_if_t<is_detected<ctx_has_match, MatchContext>::value, bool>


I think these enable_ifs are not needed anymore now that we have the match() implementation in BasicMatchContext?

Oh yeah I forgot. It's fixed now.

nikic

LGTM, but please wait a bit in case other people have more feedback.

…ternMatch (#84759) Resolves #84745. Based on SDPatternMatch introduced by #78654, this patch replaces some of basic patterns in `visitADDLike` with corresponding patterns in SDPatternMatch. This patch only replaces original folds, instead of introducing new ones.

[SelecitonDAG] Introducing SelectionDAG pattern matching framework

f22ad77

TBA...

mshockwave requested review from aemerson, bogner, nikic, RKSimon and topperc January 19, 2024 00:46

topperc reviewed Jan 19, 2024

View reviewed changes

RKSimon changed the title ~~[SelecitonDAG] Introducing the SelectionDAG pattern matching framework~~ [SelectionDAG] Introducing the SelectionDAG pattern matching framework Jan 19, 2024

RKSimon requested a review from efriedma-quic January 19, 2024 11:31

In NUses, check the subsequent pattern before checking the # of users

c884481

As SDNode::hasNUsesOfValue is pretty expensive

RKSimon mentioned this pull request Feb 4, 2024

[DAGCombiner][VP] Add DAGCombine for VP_MUL #80105

Merged

Introducing MatchContext into SDPatternMatch

c94e7a4

RKSimon reviewed Feb 13, 2024

View reviewed changes

nikic reviewed Feb 13, 2024

View reviewed changes

RKSimon requested a review from jacquesguan February 14, 2024 16:24

Address reviewer comments

5b9e51d

mshockwave added 2 commits February 14, 2024 17:00

fixup! Address reviewer comments

e47df78

Add m_Deferred

364f112

nikic reviewed Feb 16, 2024

View reviewed changes

llvm/include/llvm/CodeGen/SDPatternMatch.h Outdated Show resolved Hide resolved

llvm/include/llvm/CodeGen/SDPatternMatch.h Outdated Show resolved Hide resolved

llvm/include/llvm/CodeGen/SDPatternMatch.h Show resolved Hide resolved

mshockwave added 2 commits February 21, 2024 16:47

Address reviewer comments

5c27bc8

The major one was simplifying Node_match into Operands_match.

Provide base implementation for MatchContext::match

61229e6

nikic reviewed Feb 22, 2024

View reviewed changes

fixup! Provide base implementation for MatchContext::match

20eea8f

nikic approved these changes Feb 22, 2024

View reviewed changes

mshockwave merged commit 5874874 into llvm:main Feb 23, 2024

mshockwave deleted the patch/selectiondag-patternmatch branch February 23, 2024 19:03

RKSimon mentioned this pull request Mar 11, 2024

[DAG] DAGCombiner::visitADDLike - investigate converting fold some basic patterns to SDPatternMatch #84745

Closed

XChy mentioned this pull request Mar 11, 2024

[SelectionDAG] Replace some basic patterns in visitADDLike with SDPatternMatch #84759

Merged

[SelectionDAG] Introducing the SelectionDAG pattern matching framework #78654

[SelectionDAG] Introducing the SelectionDAG pattern matching framework #78654

Uh oh!

Conversation

mshockwave commented Jan 19, 2024

Uh oh!

mshockwave commented Jan 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RKSimon commented Jan 31, 2024

Uh oh!

RKSimon commented Feb 11, 2024

Uh oh!

mshockwave commented Feb 12, 2024

Uh oh!

mshockwave commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mshockwave commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mshockwave commented Feb 12, 2024 •

edited

Loading

mshockwave commented Feb 15, 2024 •

edited

Loading