Skip to content
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

riscv64: Improve pattern matching rules for FMA #8596

Merged
merged 2 commits into from May 12, 2024

Conversation

afonso360
Copy link
Contributor

@afonso360 afonso360 commented May 11, 2024

👋 Hey,

This PR reworks our FMA pattern matching to be slightly less verbose using the suggestions provided by @jameysharp on #8588 (comment).

It additionally adds the (fma x (splat y) z) pattern for vectors, which can be proven to be equivalent to (fma y (splat x) z). See #8588 (comment). I've only added ISA tests for these patterns since it turns out they are already present in the SIMD FMA runtests.

I think all of the patterns matched here are sound although I can't formally prove all of them. This comment #8588 (comment) describes most of the validations that I did.

This commit reworks our FMA pattern matching to be slightly less verbose. It additionally adds the `(fma x (splat y) z)` pattern for vectors, which can be proven to be equivalent to `(splat x)`.

Co-Authored-By:  Jamey Sharp <jsharp@fastly.com>
@afonso360 afonso360 requested a review from a team as a code owner May 11, 2024 11:38
@afonso360 afonso360 requested review from fitzgen and removed request for a team May 11, 2024 11:38
@afonso360 afonso360 added the cranelift:area:riscv64 Issues related to the RISC-V 64 backend. label May 11, 2024
@afonso360 afonso360 changed the title riscv64: Improve pattern matching rules for FMA. riscv64: Improve pattern matching rules for FMA May 11, 2024
@fitzgen fitzgen requested review from jameysharp and removed request for fitzgen May 11, 2024 14:08
@fitzgen
Copy link
Member

fitzgen commented May 11, 2024

Redirecting to @jameysharp since it seems like he has more context here.

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label May 11, 2024
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, I love that this worked out, and I really appreciate the effort you put into using Alive2 to check which of my guesses were correct. Too bad about signed zeroes…

By the way, this motivated me to write up #8599, which would make this kind of pattern easier to write someday.

(rule 1 (is_fneg (fneg x)) (IsFneg.Result 1 x))
(rule 0 (is_fneg x) (IsFneg.Result 0 x))

(rule 1 (lower (has_type ty (fma x_src y_src z_src)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, why does this rule need a priority?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, it shouldn't. I was debugging some stuff and forgot to remove it.

@afonso360 afonso360 enabled auto-merge May 12, 2024 21:07
@afonso360 afonso360 added this pull request to the merge queue May 12, 2024
Merged via the queue into bytecodealliance:main with commit 895a5ac May 12, 2024
22 checks passed
@afonso360 afonso360 deleted the riscv-fma-v3 branch May 12, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:riscv64 Issues related to the RISC-V 64 backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants