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

Vector128.Create regression on .NET 5 #44704

Closed
gdkchan opened this issue Nov 15, 2020 · 9 comments
Closed

Vector128.Create regression on .NET 5 #44704

gdkchan opened this issue Nov 15, 2020 · 9 comments
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI regression-from-last-release

Comments

@gdkchan
Copy link

gdkchan commented Nov 15, 2020

Description

Vector128.Create is not inserting the correct elements into the vector in some cases on .NET 5. The issue can be reproduced with the code below:

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;

namespace VectorCreateBug
{
    static class BitfieldExtensions
    {
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static int ExtractSx(this long value, int lsb, int length)
        {
            int shift = lsb & 0x3f;

            return (int)((value << (64 - (shift + length))) >> (64 - length));
        }
    }

    class Program
    {
        private struct PackedValues
        {
            private long _e0;

            public int Value0 => _e0.ExtractSx(0, 4);
            public int Value1 => _e0.ExtractSx(4, 4);
            public int Value2 => _e0.ExtractSx(8, 4);
            public int Value3 => _e0.ExtractSx(12, 4);

            public PackedValues(long packed)
            {
                _e0 = packed;
            }
        }

        static void Main(string[] args)
        {
            // Run a few times to trigger the bug due to tiered compilation...
            for (int i = 0; i < 1000; i++)
            {
                Test();
            }
        }

        static void Test()
        {
            PackedValues p = new PackedValues(0x4321);

            Vector128<int> test = Vector128.Create(p.Value0, p.Value1, p.Value2, p.Value3);

            Console.WriteLine((test.GetElement(0) + test.GetElement(1) + test.GetElement(2) + test.GetElement(3)).ToString());
        }
    }
}

It should always print 10 since 1 + 2 + 3 + 4 = 10, however when running in release mode on .NET 5, it starts printing 9 after a few iterations. This issue only happens in release mode, and only on .NET 5, it works properly on .NET Core 3.1 (I did not try older versions).

Configuration

Tested on .NET 5.0.100, OS Windows 10 x64, 9th generation Core i7 x64 CPU. I don't know if it's specific to this configuration as I didn't try on any other OS or machine yet.

Regression?

It worked properly on .NET Core 3.1, I did not try versions older than that.

Other information

One possible workaround is disabling optimizations for the method using the [MethodImpl(MethodImplOptions.NoOptimization)] attribute, but that has performance impact so it's not ideal.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 15, 2020
@EgorBo
Copy link
Member

EgorBo commented Nov 15, 2020

It's the same bug as #43569 (was fixed recently in master via #43578). A candidate for the service release?

@EgorBo EgorBo added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI regression-from-last-release labels Nov 15, 2020
@EgorBo
Copy link
Member

EgorBo commented Nov 15, 2020

Workaround for .NET5:

Vector128<int> test = Vector128.Create(p.Value0,p.Value1,p.Value2,p.Value3);

// don't use calls as arguments for Vector_.Create directly and introduce temp variables instead:

var v0 = p.Value0;
var v1 = p.Value1;
var v2 = p.Value2;
var v3 = p.Value3;
Vector128<int> test = Vector128.Create(v0,v1,v2,v3);

@marysaka
Copy link

Workaround for .NET5:

Vector128<int> test = Vector128.Create(p.Value0,p.Value1,p.Value2,p.Value3);

// don't use calls as arguments for Vector_.Create directly and introduce temp variables instead:

var v0 = p.Value0;
var v1 = p.Value1;
var v2 = p.Value2;
var v3 = p.Value3;
Vector128<int> test = Vector128.Create(v0,v1,v2,v3);

That is a better looking workaround that what I did, might give it a shot 👍

marysaka pushed a commit to Ryujinx/Ryujinx that referenced this issue Nov 15, 2020
* infra: Migrate to .NET 5

This migrate projects and CI to .NET 5

* Remove language version restrictions (now on 9.0 by default)

* infra: pin .NET 5 to avoid later issues

* infra: Cleanup csproj files

* infra: update dependencies

* infra: Add temporary workaround for a bug in Vector128.Create

see dotnet/runtime#44704 for more informations
@JulieLeeMSFT
Copy link
Member

@Thog Please let us know how the workaround goes.

@AndyAyersMS
Copy link
Member

@EgorBo you're sure this is a dup of #43569?

@EgorBo
Copy link
Member

EgorBo commented Nov 20, 2020

@EgorBo you're sure this is a dup of #43569?

Yes, the workaround I posted in #44704 (comment) works for the original repro #44704 (comment)

Vector128<int> test = Vector128.Create(p.Value0, p.Value1, p.Value2, p.Value3);
these ValueX are propertes == methods. Also, the issue doesn't reproduce on net6.

@marysaka
Copy link

@Thog Please let us know how the workaround goes.

I ended up not using the solution proposed by @EgorBo and kept the original workaround @gdkchan made when hitting the issue:
Ryujinx/Ryujinx@aa129fd#diff-9a4c528fa9a8d10cb90c5d5cb0004e77d54bc62bbf3b17a91f99b75d02901aa2R59

@AndyAyersMS AndyAyersMS removed needs more info untriaged New issue has not been triaged by the area owner labels Nov 23, 2020
@AndyAyersMS
Copy link
Member

Closing as dup of #43569.

Fix will be in 5.0.1.

qurious-pixel pushed a commit to qurious-pixel/Ryujinx that referenced this issue Dec 1, 2020
* infra: Migrate to .NET 5

This migrate projects and CI to .NET 5

* Remove language version restrictions (now on 9.0 by default)

* infra: pin .NET 5 to avoid later issues

* infra: Cleanup csproj files

* infra: update dependencies

* infra: Add temporary workaround for a bug in Vector128.Create

see dotnet/runtime#44704 for more informations
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI regression-from-last-release
Projects
None yet
Development

No branches or pull requests

6 participants