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

[JIT] X64 - Three instruction replacement sequence for multiply in certain cases #76981

Merged
merged 25 commits into from
Oct 20, 2022
Merged

[JIT] X64 - Three instruction replacement sequence for multiply in certain cases #76981

merged 25 commits into from
Oct 20, 2022

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Oct 13, 2022 •
edited
Loading

Description

Will resolve a subset of: #75119

The changes handle integer multiplication cases when a constant is one away from being the power of 2.
Some performance results are here.

Examples:

        [MethodImpl(MethodImplOptions.NoInlining)]
        static ulong UInt64_MultiplyWith15(ulong value)
        {
            // X64:      mov [[REG0:[a-z]+]], [[REG1:[a-z]+]]
            // X64-NEXT: shl [[REG0]], 4
            // X64-NEXT: sub [[REG0]], [[REG1]]
            return value * 15;
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        static ulong UInt64_MultiplyWith17(ulong value)
        {
            // X64:      mov [[REG0:[a-z]+]], [[REG1:[a-z]+]]
            // X64-NEXT: shl [[REG0]], 4
            // X64-NEXT: add [[REG0]], [[REG1]]
            return value * 17;
        }

These can technically be further optimized with lea I believe, but I'm going to do those in a separate PR.

I made very trivial updates to SuperFileCheck since I encountered a bug when writing the disasm tests for this.

Acceptance Criteria

  • Disasm tests

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 13, 2022
@ghost ghost assigned TIHan Oct 13, 2022
@ghost
Copy link

ghost commented Oct 13, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Experimenting based on this issue #75119

Author: TIHan
Assignees: TIHan
Labels:

area-CodeGen-coreclr
Milestone: -

@runfoapp runfoapp bot mentioned this pull request Oct 13, 2022
Closed
@build-analysis build-analysis bot mentioned this pull request Oct 13, 2022
Closed
@TIHan TIHan changed the title Using 3 instruction sequence for x64 multiply [JIT] X64 - Three instruction replacement sequence for multiply in certain cases Oct 14, 2022
@TIHan
Copy link
Contributor Author

TIHan commented Oct 14, 2022

This is ready. @dotnet/jit-contrib - @kunalspathak PTAL

@TIHan TIHan marked this pull request as ready for review October 14, 2022 23:38
@TIHan TIHan mentioned this pull request Oct 17, 2022
Merged
1 task
@build-analysis build-analysis bot mentioned this pull request Oct 18, 2022
Closed
2 tasks
@TIHan
Copy link
Contributor Author

TIHan commented Oct 18, 2022 •
edited
Loading

@dotnet/jit-contrib ping again, this is ready and is passing CI, diffs

There is a slight size increase, but based on the perfscore, and benchmarks mentioned in the description, execution time is faster.

@kunalspathak
Copy link
Member

kunalspathak commented Oct 19, 2022 via email

kunalspathak
kunalspathak approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

src/coreclr/jit/lowerxarch.cpp Outdated Show resolved Hide resolved
src/tests/JIT/opt/Multiply/IntMultiply.csproj Outdated Show resolved Hide resolved
@TIHan
Feedback. Removed use of FULL-LINE as it is more readable not strictl…
214625f 
…y necessary. Forgot to add an additional instruction to a disasm test.
@TIHan
Formatting
75313fb
@TIHan
Copy link
Contributor Author

TIHan commented Oct 20, 2022

CI did pass except for formatting and a timeout issue. Merging now.

@TIHan TIHan merged commit ccd9d16 into dotnet:main Oct 20, 2022
@TIHan TIHan deleted the mul-opt-x64 branch October 20, 2022 01:16
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@markples markples markples left review comments

@tannergooding tannergooding tannergooding left review comments

@kunalspathak kunalspathak kunalspathak approved these changes

Assignees

@TIHan TIHan

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TIHan @kunalspathak @tannergooding @markples