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.

Sign up for GitHub

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

Jump to bottom

Combine (shl (and x, imm1), imm2) to (shl x, imm2) #246

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Combine (shl (and x, imm1), imm2) to (shl x, imm2) #246

merged 1 commit into from
Jan 9, 2025

Conversation

abhinay-anubola
Copy link
Collaborator

@abhinay-anubola abhinay-anubola commented Dec 4, 2024 •
edited
Loading

  • Generic combiner that combines

    %t = G_AND %x, imm1
%root = G_SHL %t, imm2

    into

    %root = G_SHL %x, imm2

    Where (~imm1 << imm2) = 0

  • Updated existing tests and added new tests accordingly.

@andcarminati
Copy link
Collaborator

It will be nice to have MIR tests applying the exact pattern in terms of G_MIR instructions.

@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch from 84fedf7 to 0a0451b Compare December 10, 2024 06:44
@abhinay-anubola abhinay-anubola marked this pull request as ready for review December 10, 2024 06:45
@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch from 0a0451b to f6001f9 Compare December 11, 2024 05:20
@andcarminati
Copy link
Collaborator

Hi @abhinay-anubola, the implementation looks good! Do you have some impact measurement (QoR)?

@abhinay-anubola
Copy link
Collaborator Author

There are no significant changes, all changes are around than 0.1%

@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch from f6001f9 to cbd2aa7 Compare December 16, 2024 11:40
@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch 2 times, most recently from bc486a2 to c880e19 Compare December 17, 2024 08:12
@abhinay-anubola abhinay-anubola changed the title Combine (shl (and x, 2^n-1), n) to (shl x, n) Combine (shl (and x, imm1), imm2) to (shl x, imm2) Dec 17, 2024
@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch from c880e19 to d2fd682 Compare December 18, 2024 06:39
@andcarminati
Copy link
Collaborator

I would like to suggest updating the commit message to include the conditions that must be met in order to apply the combiner. It could be similar to what you mentioned in your initial comment. This will be helpful for future reference. The PR is in very good shape!

@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch from d2fd682 to 3b1d2f3 Compare December 18, 2024 09:48
@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch from 3b1d2f3 to 7a5eb73 Compare January 2, 2025 10:54
@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch 4 times, most recently from 046edfc to 6829573 Compare January 7, 2025 14:25
@abhinay-anubola abhinay-anubola force-pushed the sanubola.G_SHL.G_AND.combine branch from 6829573 to 3805da6 Compare January 8, 2025 10:44
martien-de-jong
martien-de-jong reviewed Jan 9, 2025
View reviewed changes
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
// Try to match shl (and x, imm1), imm2
int64_t ShiftImm, AndImm;
if (!mi_match(DstReg, MRI,
m_GShl(m_OneNonDBGUse(m_GAnd(m_Reg(Reg), m_ICst(AndImm))),
Copy link
Collaborator

@martien-de-jong martien-de-jong Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I don't think the mOneNonDBGUse condition is tested, but I also don't think it's necessary. We just remove a use of the AND. If it's the last use, DCE will remove it; if it's not, it will serve the other uses.

martien-de-jong
martien-de-jong approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@martien-de-jong martien-de-jong left a comment

Choose a reason for hiding this comment

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

Would be nice if this could be upstreamed to get the AArch64 and AMDGPU updates in mainstream. That would require to isolate the AIE changes in a separate commit.

@abhinay-anubola abhinay-anubola merged commit 85e2abc into aie-public Jan 9, 2025
8 checks passed
@abhinay-anubola abhinay-anubola deleted the sanubola.G_SHL.G_AND.combine branch January 9, 2025 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krishnamtibrewala krishnamtibrewala krishnamtibrewala left review comments

@andcarminati andcarminati andcarminati left review comments

@martien-de-jong martien-de-jong martien-de-jong approved these changes

@abnikant abnikant Awaiting requested review from abnikant abnikant is a code owner

@gbossu gbossu Awaiting requested review from gbossu gbossu is a code owner

@khallouh khallouh Awaiting requested review from khallouh khallouh is a code owner

@konstantinschwarz konstantinschwarz Awaiting requested review from konstantinschwarz konstantinschwarz is a code owner

@SagarMaheshwari99 SagarMaheshwari99 Awaiting requested review from SagarMaheshwari99

@stephenneuendorffer stephenneuendorffer Awaiting requested review from stephenneuendorffer stephenneuendorffer is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abhinay-anubola @andcarminati @martien-de-jong @krishnamtibrewala