Skip to content

Update Adler32 to correctly filter intrinsics. #1229

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
Merged
merged 3 commits into from
Jun 15, 2020
Merged

Update Adler32 to correctly filter intrinsics. #1229

merged 3 commits into from
Jun 15, 2020

Conversation

@JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Jun 15, 2020 •
edited
Loading

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Introduces the correct intrinsics support check in the enhanced Adler32 implementation. Fix #1228

@antonfirsov if you have any idea how to test this please let me know if you have any thoughts.

@JimBobSquarePants JimBobSquarePants added this to the 1.0.0 milestone Jun 15, 2020
@codecov
Copy link

codecov bot commented Jun 15, 2020 •
edited
Loading

Codecov Report

Merging #1229 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1229   +/-   ##
=======================================
  Coverage   82.54%   82.54%           
=======================================
  Files         697      697           
  Lines       30512    30512           
  Branches     3469     3469           
=======================================
  Hits        25187    25187           
  Misses       4605     4605           
  Partials      720      720
Flag Coverage Δ
#unittests 82.54% <100.00%> (ø)
Impacted Files Coverage Δ
src/ImageSharp/Formats/Png/Zlib/Adler32.cs 99.09% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0890bca...e0d705a. Read the comment docs.

@antonfirsov
Copy link
Member

antonfirsov commented Jun 15, 2020 •
edited
Loading

We can disable runtime features with environment variables , and run some Adler32 tests in external process. (processStartInfo.Environment["COMPlus_EnableSSSE3"] = "0" + feed processStartInfo to RemoteExecutor)
A list of variables that might be interesting for our code:
https://github.com/dotnet/runtime/blob/f987b3bc8f8a4d49ca1fc8821a48a7614dfee7cd/src/coreclr/tests/testenvironment.proj#L15-L33

I would also add Assert.False(Ssse3.IsSupported) to the test, to make sure the JIT knob works.

@tannergooding if you are around: do you think this is a reliable approach?

@tannergooding
Copy link
Contributor

tannergooding commented Jun 15, 2020

That is a reliable way to test the software fallback path and is what we do in the runtime (it's also why we exposed these knobs).

dlemstra
dlemstra reviewed Jun 15, 2020
View reviewed changes
src/ImageSharp/Formats/Png/Zlib/Adler32.cs
if (Sse3.IsSupported && buffer.Length >= MinBufferSize)
if (Ssse3.IsSupported && buffer.Length >= MinBufferSize)
{
return CalculateSse(adler, buffer);
Copy link
Member

@dlemstra dlemstra Jun 15, 2020

Choose a reason for hiding this comment

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

Should we also rename CalculateSse to CalculateSsse?

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Jun 15, 2020

Choose a reason for hiding this comment

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

I would say no. It’s still an iteration of SSE technology.

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Jun 15, 2020

@antonfirsov @tannergooding Test added now. Think I've got it right.

antonfirsov
antonfirsov approved these changes Jun 15, 2020
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM, good job!

tannergooding
tannergooding reviewed Jun 15, 2020
View reviewed changes
tests/ImageSharp.Tests/TestUtilities/TestEnvironment.Features.cs Outdated
public const string EnableBMI2 = "COMPlus_EnableBMI2";
public const string EnableFMA = "COMPlus_EnableFMA";
public const string EnableHWIntrinsic = "COMPlus_EnableHWIntrinsic";
public const string EnableIncompleteISAClass = "COMPlus_EnableIncompleteISAClass";
Copy link
Contributor

@tannergooding tannergooding Jun 15, 2020

Choose a reason for hiding this comment

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

This one isn't a production switch.

For the others, there is a rough hierarchy they follow:

  • EnableHWIntrinsic
    • EnableSSE
      • EnableSSE2
        • EnableAES
        • EnablePCLMULQDQ
        • EnableSSE3
          • EnableSSSE3
            • EnableSSE41
              • EnableSSE42
                • EnablePOPCNT
                • EnableAVX
                  • EnableFMA
                  • EnableAVX2
    • EnableBMI1
    • EnableBMI2
    • EnableLZCNT

FeatureSIMD ends up impacting all SIMD support (including System.Numerics) but not things like LZCNT, BMI1, or BMI2
EnableSSE3_4 is a legacy switch that exists for compat and is basically the same as EnableSSE3

Copy link
Contributor

@tannergooding tannergooding Jun 15, 2020

Choose a reason for hiding this comment

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

The hierarchy basically means disabling SSE (EnableSSE=0) will also disable everything below it

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Jun 15, 2020

Choose a reason for hiding this comment

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

Ah lovely. I'll reference that. I intend on doing more intrinsics work in the near future.

@JimBobSquarePants JimBobSquarePants merged commit cecbe7c into master Jun 15, 2020
@JimBobSquarePants JimBobSquarePants deleted the js/fix-1228 branch June 15, 2020 23:23
This was referenced Jul 30, 2025
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
@dependabot dependabot bot mentioned this pull request Sep 23, 2025
Open
This was referenced Oct 3, 2025
Open
Merged
Closed
Open
@dependabot dependabot bot mentioned this pull request Oct 19, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlemstra dlemstra dlemstra left review comments

@tocsoft tocsoft tocsoft approved these changes

@antonfirsov antonfirsov antonfirsov approved these changes

+1 more reviewer

@tannergooding tannergooding tannergooding left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug formats:png

Projects

None yet

Milestone

1.0.0

Development

Successfully merging this pull request may close these issues.

PNG encoder throws an exception on x64

6 participants

@JimBobSquarePants @antonfirsov @tannergooding @tocsoft @dlemstra