Skip to content

Better ways to write GlobalISel combine "match" and "apply" functions #92410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jayfoad opened this issue May 16, 2024 · 1 comment · Fixed by #135941
Closed

Better ways to write GlobalISel combine "match" and "apply" functions #92410

jayfoad opened this issue May 16, 2024 · 1 comment · Fixed by #135941
Assignees

Comments

@jayfoad
Copy link
Contributor

jayfoad commented May 16, 2024

GlobalISel combines are written as separate "match" and "apply" functions but since #92239 we try to inline those into a single "match+apply" function. The two parts can share information in a couple of ways:

  1. You can declare a matchinfo (aka matchdata) type, an instance of which is populated by the match function and can be used by the apply function. This adds a bit of boilerplate to the tablegen declarations and is slightly less ergonomic than allowing the apply function to refer to the match function's locals directly.

  2. (Really a special case of 1) the match function can set the matchdata to be a lambda which is the real apply function. In tablegen the match function is declared to be Helper.applyBuildFn which is a helper that just invokes the lambda. This has the advantage that the apply function can refer to (captured) locals from the match function directly.

The problem with (2) is that when we try to compile the combined match+apply function, I think the compiler will not be able to see throughHelper.applyBuildFn in order to inline the real apply function.

I would like to find a way to write combines that is as ergonomic and as efficient as possible.

One idea I had was to invent some tablegen syntax that lets you declare a single combined match+apply function that you write explicitly in C++, instead of writing the two parts separately.

@Pierre-vh
Copy link
Contributor

I want to give this a shot, my idea is to add a new (combine ...) pattern so one can write

def some_combine : GICombineRule<
  (defs root:$root),
  (combine (G_ZEXT $tmp, $ext),
                  (G_STORE $tmp, $ptr):$root),
                  [{ return doSomeCombine(); }]>;

Such CombineRules would have:

  • No match data
  • One combine operation containing
    • (optional) MIR pattern to match instructions, or a wip_match_opcode.
    • At least one C++ code fragment (that can match/mutate/do whatever as long as it returns true if anything changed).

I'll have to refactor the internals of the emitter a bit to make it fit cleanly (because a lot of it is match->apply oriented).

I do wonder if I should bother supporting both match/apply patterns in any case other than MIR patterns though?
If we have

def OneMatchOneApply : GICombineRule<
  (defs root:$a),
  (match (G_FABS $a, $b), "return MATCH0;"),
  (apply "APPLY0")>;

Should this be an error and force us to write it as a single (combine ...) rule if the "apply" only has C++?

It'd be a lot to migrate but then at least we'd enforce one consistent style all over the place. The patch would be a bit of a pain to review though with all of the combine rewrites + the backend change.

@Pierre-vh Pierre-vh self-assigned this Apr 15, 2025
Pierre-vh added a commit that referenced this issue Apr 16, 2025
Adds a `combine` action (DAG operator) which allows for easy definition of
combine rule that only match one or more instructions, and defer all remaining
match/apply logic to C++ code.

This avoids the need for split match/apply function in such cases. One function
can do the trick as long as it returns `true` if it changed any code.

This is implemented as syntactic sugar over match/apply. The combine rule is
just a match pattern BUT every C++ pattern inside is treated as an "apply" function.
This makes it fit seamlessly with the current backend.

Fixes #92410
Pierre-vh added a commit that referenced this issue Apr 22, 2025
Adds a `combine` action (DAG operator) which allows for easy definition of
combine rule that only match one or more instructions, and defer all remaining
match/apply logic to C++ code.

This avoids the need for split match/apply function in such cases. One function
can do the trick as long as it returns `true` if it changed any code.

This is implemented as syntactic sugar over match/apply. The combine rule is
just a match pattern BUT every C++ pattern inside is treated as an "apply" function.
This makes it fit seamlessly with the current backend.

Fixes #92410
Pierre-vh added a commit that referenced this issue Apr 25, 2025
Adds a `combine` action (DAG operator) which allows for easy definition of
combine rule that only match one or more instructions, and defer all remaining
match/apply logic to C++ code.

This avoids the need for split match/apply function in such cases. One function
can do the trick as long as it returns `true` if it changed any code.

This is implemented as syntactic sugar over match/apply. The combine rule is
just a match pattern BUT every C++ pattern inside is treated as an "apply" function.
This makes it fit seamlessly with the current backend.

Fixes #92410
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
Adds a `combine` action (DAG operator) which allows for easy definition of
combine rule that only match one or more instructions, and defer all remaining
match/apply logic to C++ code.

This avoids the need for split match/apply function in such cases. One function
can do the trick as long as it returns `true` if it changed any code.

This is implemented as syntactic sugar over match/apply. The combine rule is
just a match pattern BUT every C++ pattern inside is treated as an "apply" function.
This makes it fit seamlessly with the current backend.

Fixes llvm#92410
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
Adds a `combine` action (DAG operator) which allows for easy definition of
combine rule that only match one or more instructions, and defer all remaining
match/apply logic to C++ code.

This avoids the need for split match/apply function in such cases. One function
can do the trick as long as it returns `true` if it changed any code.

This is implemented as syntactic sugar over match/apply. The combine rule is
just a match pattern BUT every C++ pattern inside is treated as an "apply" function.
This makes it fit seamlessly with the current backend.

Fixes llvm#92410
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
Adds a `combine` action (DAG operator) which allows for easy definition of
combine rule that only match one or more instructions, and defer all remaining
match/apply logic to C++ code.

This avoids the need for split match/apply function in such cases. One function
can do the trick as long as it returns `true` if it changed any code.

This is implemented as syntactic sugar over match/apply. The combine rule is
just a match pattern BUT every C++ pattern inside is treated as an "apply" function.
This makes it fit seamlessly with the current backend.

Fixes llvm#92410
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this issue May 9, 2025
Adds a `combine` action (DAG operator) which allows for easy definition of
combine rule that only match one or more instructions, and defer all remaining
match/apply logic to C++ code.

This avoids the need for split match/apply function in such cases. One function
can do the trick as long as it returns `true` if it changed any code.

This is implemented as syntactic sugar over match/apply. The combine rule is
just a match pattern BUT every C++ pattern inside is treated as an "apply" function.
This makes it fit seamlessly with the current backend.

Fixes llvm#92410
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants