Skip to content

disable optimizations for PopCount #99796

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

Merged
merged 2 commits into from
Mar 15, 2024
Merged

disable optimizations for PopCount #99796

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1492b8c
disable optimizations for PopCount
mangod9 Mar 15, 2024
eadc004
remove whitespace for jit-format
mangod9 Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/coreclr/jit/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3436,6 +3436,11 @@ uint32_t BitOperations::Log2(uint64_t value)
// Return Value:
// The population count (number of bits set) of value
//
#if defined(_MSC_VER)
Copy link
Member Author

Choose a reason for hiding this comment

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

note that this disables inlining of these methods too, so we might need to disable the specific optimization if this is perf critical.

// Disable optimizations for PopCount to avoid the compiler from generating intrinsics
// not supported on all platforms.
#pragma optimize("", off)
Copy link
Member

Choose a reason for hiding this comment

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

Have we root caused and found a minimal repro for this?

Given popcnt is not a x64 baseline instruction, it should not be getting generated without explicit opt in (meaning either we have a cmake/config issue on our end or MSVC has a bug)

Copy link
Member

Choose a reason for hiding this comment

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

If it's an MSVC issue, we should ensure an appropriate issue is filed against the compiler to track them fixing this.

Alternatively, if Windows has changed requirements at all, we should ensure it's documented and appropriately take advantage of these requirements (I vaguely remember seeing some change where Windows 11 may require SSE4.2 instructions)

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, if Windows has changed requirements at all, we should ensure it's documented and appropriately take advantage of these requirements (I vaguely remember seeing some change where Windows 11 may require SSE4.2 instructions)

IIRC next update of Windows 11 requires popcnt, however I'd say that dotnet shouldn't require it as it still supports 10 and Server 2012.

Copy link
Member Author

Choose a reason for hiding this comment

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

we should be able to build a repro. I have verified within the debugger that the compiler does generate popcnt, and as Egor points out above looks like a fix has been made recently.

Copy link
Member

Choose a reason for hiding this comment

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

If it has been fixed already, you should be able to put the workaround under _MSC_VER or _MSC_FULL_VER ifdef to make self-documenting and self-healing.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah checking if we can just pickup the tool update.

IIRC next update of Windows 11 requires popcnt, however I'd say that dotnet shouldn't require it as it still supports 10 and Server 2012.

Correct Win11 24H2 is expected to require it, but yeah that is a separate discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it has been fixed already, you should be able to put the workaround under _MSC_VER or _MSC_FULL_VER ifdef to make self-documenting and self-healing.

makes sense, but we will need to check the exact versions which had the issue (since it affected both 17.8 and 17.9).

Copy link
Member

Choose a reason for hiding this comment

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

According to the links I shared:

has been fixed in 17.8.6 and 17.9.0 Preview 5

Copy link
Member Author

Choose a reason for hiding this comment

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

since this is a temporary change, not worth special casing versions in case we get the numbers wrong. The toolset update is going to be in the next few weeks and I can create a revert PR.

#endif // _MSC_VER
uint32_t BitOperations::PopCount(uint32_t value)
{
#if defined(_MSC_VER)
Expand Down Expand Up @@ -3488,6 +3493,9 @@ uint32_t BitOperations::PopCount(uint64_t value)
return static_cast<uint32_t>(result);
#endif
}
#if defined(_MSC_VER)
#pragma optimize("", on)
#endif // _MSC_VER

//------------------------------------------------------------------------
// BitOperations::ReverseBits: Reverses the bits in an integer value
Expand Down