Ensure Max/Min for floating-point on x86/x64 are not handled as commutative #75683
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis resolves the ImageSharp bug tracked by SixLabors/ImageSharp#2117 This is a decently old bug and was introduced about 5 years ago in: dotnet/coreclr@654a8d5 This means that it exists in .NET 3.1, .NET 5 (out of support), and .NET 6. We should "at minimum" backport this to .NET 7 RTM. We should also consider backporting to .NET 6 since it is LTS and still has a couple years of support (until Dec 2024). I don't think this is needed for .NET Core 3.1 since it goes out of support in December.
|
| if (HWIntrinsicInfo::IsMaybeCommutative(id)) | ||
| { | ||
| switch (id) | ||
| { | ||
| case NI_SSE_Max: | ||
| case NI_SSE_Min: | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| case NI_SSE2_Max: | ||
| case NI_SSE2_Min: | ||
| { | ||
| return !varTypeIsFloating(node->GetSimdBaseType()); | ||
| } | ||
|
|
||
| case NI_AVX_Max: | ||
| case NI_AVX_Min: | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| default: | ||
| { | ||
| unreached(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I've kept this "simple" since we need to backport into .NET 7.
It isn't just a "return false", however, because we will want to specially handle the case in .NET 8 since certain scenarios can be commutative and this makes the follow up PR significantly simpler.
| #endif // TARGET_XARCH | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Notably Arm64 is returning constant false and it shouldn't be. Given this needs to be backported to .NET 7, I didn't "fix" this (its not a correctness bug, just a case where we might generate "worse" codegen).
I plan on putting up a follow up PR to ensure Arm64 intrinsics are handled as commutative (they are already correctly marked as required, this function returning false is the blocker for them)
|
superpmi is a failure to upload logs for the x86 job. |
|
/backport to release/7.0-rc2 |
|
Started backporting to release/7.0-rc2: https://github.com/dotnet/runtime/actions/runs/3069189452 |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/3070029740 |
|
@tannergooding backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Adding a regression test for sixlabors/imagesharp#2117
Applying: Ensure Max/Min for floating-point on x86/x64 are not handled as commutative
Using index info to reconstruct a base tree...
M src/coreclr/jit/gentree.cpp
M src/coreclr/jit/hwintrinsic.h
M src/coreclr/jit/hwintrinsiclistxarch.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiclistxarch.h
Auto-merging src/coreclr/jit/hwintrinsic.h
Auto-merging src/coreclr/jit/gentree.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/gentree.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Ensure Max/Min for floating-point on x86/x64 are not handled as commutative
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
…tative (dotnet#75683) * Adding a regression test for SixLabors/ImageSharp#2117 * Ensure Max/Min for floating-point on x86/x64 are not handled as commutative * Applying formatting patch
…ndled as commutative (#75772) * Ensure Max/Min for floating-point on x86/x64 are not handled as commutative (#75683) * Adding a regression test for SixLabors/ImageSharp#2117 * Ensure Max/Min for floating-point on x86/x64 are not handled as commutative * Applying formatting patch * Directly access the gtHWIntrinsicId field since the helper method doesn't exist in .NET 6 * Fixing a regression test to use the GetElement API available in .NET 6
ghost
locked as resolved and limited conversation to collaborators
This resolves the ImageSharp bug tracked by SixLabors/ImageSharp#2117
This is a decently old bug and was introduced about 5 years ago in: dotnet/coreclr@654a8d5
This means that it exists in .NET 3.1, .NET 5 (out of support), and .NET 6.
We should "at minimum" backport this to .NET 7 RTM. We should also consider backporting to .NET 6 since it is LTS and still has a couple years of support (until Dec 2024). I don't think this is needed for .NET Core 3.1 since it goes out of support in December.