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.

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

Vector<T> operator+ does not generate NotSupportedException on arm32. #37506

Closed
sandreenko opened this issue Jun 5, 2020 · 9 comments · Fixed by #38241
Closed

Vector<T> operator+ does not generate NotSupportedException on arm32. #37506

sandreenko opened this issue Jun 5, 2020 · 9 comments · Fixed by #38241
Assignees
Labels
arch-arm32 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug
Milestone

Comments

@sandreenko
Copy link
Contributor

sandreenko commented Jun 5, 2020

We are expecting the following test to pass:

    public static int Main()
    {
        var a = new Vector<Vector4>(new Vector4(1));
        try
        {
            a = a + a;
            Debug.Assert(false, "unreachable");
        }
        catch (System.NotSupportedException)
        {
        }
        return 100;
    }

but on arm32 it hits the assert, based on @tannergooding analysis it happens because on arm32 we don't have IsHardwareAccelerated, so we go to the else condition in Vector<T> operator +:
https://source.dot.net/#System.Private.CoreLib/Vector.cs,729
and the else branch is missing something like (in my understanding):

    else
    {
        throw new NotSupportedException(SR.Arg_TypeNotSupported);
    }

https://source.dot.net/#System.Private.CoreLib/Vector.cs,826
similar to the IsHardwareAccelerated block.

cc @CarolEidt

JitDump confirms that we are not doing addition and just generate `default:
Addition.txt

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Jun 5, 2020
@tannergooding tannergooding added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jun 5, 2020
@tannergooding
Copy link
Member

CC. @jeffhandley, this is a breaking-change but one we should take as we get two different behaviors based on whether SIMD support is enabled or disabled.

You should also be able to hit this scenario on x86/x64 or ARM64 by setting COMPlus_FeatureSIMD=0.

When IsHardwareAccelerated is false, the operation will just return default as will several of the other operators.

@jeffhandley
Copy link
Member

Thanks, @tannergooding. Can you follow our usual process for documenting this as a 5.0 breaking change? And I presume we would not back-port this fix to 3.x; does that match your expectation?

@BruceForstall
Copy link
Member

@tannergooding @CarolEidt @sandreenko why would we expect Vector to work? I thought Vector<T> only supported primitive number types T: https://docs.microsoft.com/en-us/dotnet/api/system.numerics.vector-1?view=net-5.0

@BruceForstall BruceForstall added this to the 5.0 milestone Jun 8, 2020
@BruceForstall BruceForstall removed the untriaged New issue has not been triaged by the area owner label Jun 8, 2020
@tannergooding
Copy link
Member

tannergooding commented Jun 8, 2020

We don't expect it to work, we expect it to throw a NotSupportedException. Today it is "working" and returning a default, but only on platforms or scenarios where IsHardwareAccelerated is returning false

@tannergooding
Copy link
Member

As per the above...

Vector<T> is generic and is meant to only support the following 10 primitive types: byte, sbyte, short, ushort, int, uint, long, ulong, float, and double and to throw NotSupportedException for any other generic type.

For the majority of scenarios this is the case, however there are some scenarios such as ARM32 (where hardware acceleration is not supported) or setting COMPlus_FeatureSIMD=0 where a subset of operations return default rather than throwing a NotSupportedException.

We wouldn't bring this for 3.x because:

  • It is a breaking change
  • We don't have any customers reporting the issue
  • The issue should be basically non-existent for platforms supported in 3.x as it requires you to be using ARM32 or to have explicitly disabled the HWAcceleration which is enabled by default on x86, x64, and Arm64

We want to fix this in 5.0 because:

  • The existing behavior is inconsistent and goes against the original design goals of the types
  • The existing behavior may prevent or make supporting additional types (like half, nint, or nuint) more difficult in the future

CC. @marklio and @PriyaPurkayastha for approval.

@marklio
Copy link

marklio commented Jun 8, 2020

This seems reasonable. I'd add a couple of things for framing the impact of the break (correct if I'm wrong):

  • The behavior isn't "correct" on arm32, so we're not breaking "working" functionality so much as making unsupported behavior more detectable/diagnosable. (note this doesn't mean it's not a breaking change, just a different class of changes that I think has a lower bar)
  • The breaking scenarios are edge case. Either arm32, or "turning off" SIMD. (this is covered in the bullets for why we wouldn't bother fixing it for 3.x, but I think it helps make a case that this isn't very breaking in 5.0)

@tannergooding
Copy link
Member

Both points are correct 😄

@BruceForstall
Copy link
Member

@tannergooding Should you own this issue?

@tannergooding tannergooding self-assigned this Jun 22, 2020
@tannergooding
Copy link
Member

Yes. I've assigned and will work on getting a fix up.

sandreenko pushed a commit to sandreenko/runtime that referenced this issue Jul 7, 2020
sandreenko pushed a commit that referenced this issue Jul 8, 2020
* Reenable GitHub_26491.

Closes #13355

* Reenable crossgen2 tests failing with old retyping/

They were fixed both with and without retyping.
Closes #37883.

* Reenable HVA merge cases.

Closes #37341, closes #37880.

* Reenable GitHub_35821.

Closes #36206, closes #36418.

The issue was fixed by #37499.

* Delete extra lines that are no longer needed.

#37506 was fixed in #38241.

* delete a throwing init.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm32 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug
Projects
None yet
6 participants