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.

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

Mark DivRem with RequiresPreviewFeatures #82221

Merged
merged 4 commits into from
Feb 16, 2023
Merged

Mark DivRem with RequiresPreviewFeatures #82221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e8139a2
Add back DivRem in ref
kunalspathak Feb 16, 2023
3b132e8
Remove fakelib from test projects
kunalspathak Feb 16, 2023
a7fdda1
Mark APIs with RequiresPreviewFeatures
kunalspathak Feb 16, 2023
2be2f38
Revert compatibility changes in SPC
kunalspathak Feb 16, 2023
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
21 changes: 0 additions & 21 deletions src/coreclr/System.Private.CoreLib/CompatibilitySuppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,3 @@
<?xml version="1.0" encoding="utf-8"?>
<!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Internal.Console</Target>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:System.Runtime.CompilerServices.ICastable</Target>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Resources.ResourceManager.BaseNameField</Target>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Resources.ResourceSet.Reader</Target>
</Suppression>
<Suppression>
<DiagnosticId>CP0014</DiagnosticId>
<Target>M:System.Runtime.InteropServices.Marshal.CreateWrapperOfType(System.Object,System.Type)-&gt;object?:[T:System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute]</Target>
</Suppression>
</Suppressions>
16 changes: 0 additions & 16 deletions src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ExtraApis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,3 @@ public static partial class Debug
public static System.Diagnostics.DebugProvider SetProvider(System.Diagnostics.DebugProvider provider) { throw null; }
}
}
namespace System.Runtime.Intrinsics.X86
{
public abstract partial class X86Base
{
public abstract partial class X64
{
public static (ulong Quotient, ulong Remainder) DivRem(ulong lower, ulong upper, ulong divisor) { throw null; }
public static (long Quotient, long Remainder) DivRem(ulong lower, long upper, long divisor) { throw null; }
}

public static (uint Quotient, uint Remainder) DivRem(uint lower, uint upper, uint divisor) { throw null; }
public static (int Quotient, int Remainder) DivRem(uint lower, int upper, int divisor) { throw null; }
public static (nuint Quotient, nuint Remainder) DivRem(nuint lower, nuint upper, nuint divisor) { throw null; }
public static (nint Quotient, nint Remainder) DivRem(nuint lower, nint upper, nint divisor) { throw null; }
}
}
6 changes: 0 additions & 6 deletions src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ExtraApis.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,3 @@ T:System.Runtime.Serialization.DeserializationToken
M:System.Runtime.Serialization.SerializationInfo.StartDeserialization
T:System.Diagnostics.DebugProvider
M:System.Diagnostics.Debug.SetProvider(System.Diagnostics.DebugProvider)
M:System.Runtime.Intrinsics.X86.X86Base.X64.DivRem(System.UInt64 lower, System.UInt64 upper, System.UInt64 divisor)
M:System.Runtime.Intrinsics.X86.X86Base.X64.DivRem(System.UInt64 lower, System.Int64 upper, System.Int64 divisor)
M:System.Runtime.Intrinsics.X86.X86Base.DivRem(uint lower, uint upper, uint divisor)
M:System.Runtime.Intrinsics.X86.X86Base.DivRem(int lower, int upper, int divisor)
M:System.Runtime.Intrinsics.X86.X86Base.DivRem(nuint lower, nuint upper, nuint divisor)
M:System.Runtime.Intrinsics.X86.X86Base.DivRem(nuint lower, nint upper, nint divisor)
7 changes: 7 additions & 0 deletions .../System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/X86Base.PlatformNotSupported.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Runtime.CompilerServices;
using System.Runtime.Versioning;

namespace System.Runtime.Intrinsics.X86
{
Expand Down Expand Up @@ -50,12 +51,14 @@ internal X64() { }
/// unsigned __int64 _udiv128(unsigned __int64 highdividend, unsigned __int64 lowdividend, unsigned __int64 divisor, unsigned __int64* remainder)
/// DIV reg/m64
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (ulong Quotient, ulong Remainder) DivRem(ulong lower, ulong upper, ulong divisor) { throw new PlatformNotSupportedException(); }

/// <summary>
/// __int64 _div128(__int64 highdividend, __int64 lowdividend, __int64 divisor, __int64* remainder)
/// DIV reg/m64
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (long Quotient, long Remainder) DivRem(ulong lower, long upper, long divisor) { throw new PlatformNotSupportedException(); }
}

Expand Down Expand Up @@ -90,21 +93,25 @@ internal X64() { }
/// <summary>
/// DIV reg/m32
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (uint Quotient, uint Remainder) DivRem(uint lower, uint upper, uint divisor) { throw new PlatformNotSupportedException(); }

/// <summary>
/// IDIV reg/m32
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (int Quotient, int Remainder) DivRem(uint lower, int upper, int divisor) { throw new PlatformNotSupportedException(); }

/// <summary>
/// IDIV reg/m
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (nuint Quotient, nuint Remainder) DivRem(nuint lower, nuint upper, nuint divisor) { throw new PlatformNotSupportedException(); }

/// <summary>
/// IDIV reg/m
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (nint Quotient, nint Remainder) DivRem(nuint lower, nint upper, nint divisor) { throw new PlatformNotSupportedException(); }

/// <summary>
Expand Down
13 changes: 7 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/X86Base.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,15 @@

using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;

namespace System.Runtime.Intrinsics.X86
{
/// <summary>
/// This class provides access to the x86 base hardware instructions via intrinsics
/// </summary>
[Intrinsic]
#if SYSTEM_PRIVATE_CORELIB
[CLSCompliant(false)]
#endif
public abstract partial class X86Base
{
internal X86Base() { }
Expand Down Expand Up @@ -52,12 +51,14 @@ internal X64() { }
/// unsigned __int64 _udiv128(unsigned __int64 highdividend, unsigned __int64 lowdividend, unsigned __int64 divisor, unsigned __int64* remainder)
/// DIV reg/m64
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (ulong Quotient, ulong Remainder) DivRem(ulong lower, ulong upper, ulong divisor) => DivRem(lower, upper, divisor);

/// <summary>
/// __int64 _div128(__int64 highdividend, __int64 lowdividend, __int64 divisor, __int64* remainder)
/// DIV reg/m64
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (long Quotient, long Remainder) DivRem(ulong lower, long upper, long divisor) => DivRem(lower, upper, divisor);
}

Expand Down Expand Up @@ -89,35 +90,35 @@ internal X64() { }
/// </summary>
public static unsafe (int Eax, int Ebx, int Ecx, int Edx) CpuId(int functionId, int subFunctionId)
{
#if SYSTEM_PRIVATE_CORELIB
int* cpuInfo = stackalloc int[4];
__cpuidex(cpuInfo, functionId, subFunctionId);
return (cpuInfo[0], cpuInfo[1], cpuInfo[2], cpuInfo[3]);
#else
return (0, 0, 0, 0);
#endif
}

/// <summary>
/// unsigned _udiv64(unsigned __int64 dividend, unsigned divisor, unsigned* remainder)
/// DIV reg/m32
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (uint Quotient, uint Remainder) DivRem(uint lower, uint upper, uint divisor) => DivRem(lower, upper, divisor);

/// <summary>
/// int _div64(__int64 dividend, int divisor, int* remainder)
/// IDIV reg/m32
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (int Quotient, int Remainder) DivRem(uint lower, int upper, int divisor) => DivRem(lower, upper, divisor);

/// <summary>
/// IDIV reg/m
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (nuint Quotient, nuint Remainder) DivRem(nuint lower, nuint upper, nuint divisor) => DivRem(lower, upper, divisor);

/// <summary>
/// IDIV reg/m
/// </summary>
[RequiresPreviewFeatures("DivRem is in preview.")]
public static (nint Quotient, nint Remainder) DivRem(nuint lower, nint upper, nint divisor) => DivRem(lower, upper, divisor);

/// <summary>
Expand Down
12 changes: 12 additions & 0 deletions src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5217,11 +5217,23 @@ public abstract partial class X86Base
internal X86Base() { }
public static bool IsSupported { get { throw null; } }
public static (int Eax, int Ebx, int Ecx, int Edx) CpuId(int functionId, int subFunctionId) { throw null; }
[System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")]
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan that we ship these in .NET 8 as preview, or is the plan that by the time we ship .NET 8 it's fully supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we plan to address it in .NET 8. Tracking issue #82194

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Technically every new API we add in previews is preview and doesn't require such attribution; this attribute was really intended to be used for things to remain in preview in supported releases. If we're just trying to scare people off from using it in previews, I guess it's ok, but it seems superfluous to me.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that this would be superfluous for ordinary APIs.

This is codegen intrinsic that may require non-trivial work to complete. Tagging it as preview makes us "ready to ship" without any additional changes, even in the case we do not have a chance to complete the work for some reason. Deleting the tag once the work is complete is a small clean change.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. Of course, we still need to remember to remove the attribute when we complete that additional work. Let's make sure to track that somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated #82194 to capture it.

public static (uint Quotient, uint Remainder) DivRem(uint lower, uint upper, uint divisor) { throw null; }
[System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")]
public static (int Quotient, int Remainder) DivRem(uint lower, int upper, int divisor) { throw null; }
[System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")]
public static (nuint Quotient, nuint Remainder) DivRem(nuint lower, nuint upper, nuint divisor) { throw null; }
[System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")]
public static (nint Quotient, nint Remainder) DivRem(nuint lower, nint upper, nint divisor) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

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

Have these APIs gone through API review?

Copy link
Member

Choose a reason for hiding this comment

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

Yes: #27292 (comment)

Copy link
Member

@jkotas jkotas Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I assume that you are asking about this specific overload - I do not see it in the approved set. All overloads are in the approved set - I was looking in a wrong place.

Copy link
Member

Choose a reason for hiding this comment

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

The general premise of extending the intrinsics to support nint/nuint was reviewed and approved already: #52021

AFAIR, we covered at the time in API review that any other already approved but NYI APIs were also covered by that.

Copy link
Member

Choose a reason for hiding this comment

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

The same general approval for nint/nuint for Arm64 was done here: #52027

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, it's covered in the approval explicitly: #27292 (comment)

public static void Pause() { throw null; }
public abstract partial class X64
{
internal X64() { }
public static bool IsSupported { get { throw null; } }
[System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")]
public static (ulong Quotient, ulong Remainder) DivRem(ulong lower, ulong upper, ulong divisor) { throw null; }
[System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")]
public static (long Quotient, long Remainder) DivRem(ulong lower, long upper, long divisor) { throw null; }
}
}

Expand Down
76 changes: 0 additions & 76 deletions src/libraries/System.Runtime.Intrinsics/src/CompatibilitySuppressions.xml
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion src/tests/JIT/HardwareIntrinsics/HardwareIntrinsics_r.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<!-- For most of these, that just involves excluding the projects when the architecture is mismatched -->
<!-- For Vector512, we only have a very small pool of machines with acceleration support, so they are always outerloop -->

<MergedWrapperProjectReference Include="*/**/*.csproj" Exclude="**/*_ro.csproj;**/*.RefOnly.csproj" />
<MergedWrapperProjectReference Include="*/**/*.csproj" Exclude="**/*_ro.csproj" />

<MergedWrapperProjectReference Remove="Arm/**/*.csproj" Condition="'$(_IncludeArm64HWIntrinsicTests)' != 'true'" />
<MergedWrapperProjectReference Remove="General/Vector64/**/*.csproj" Condition="'$(_IncludeArm64HWIntrinsicTests)' != 'true'" />
Expand Down
2 changes: 1 addition & 1 deletion src/tests/JIT/HardwareIntrinsics/HardwareIntrinsics_ro.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
<!-- For most of these, that just involves excluding the projects when the architecture is mismatched -->
<!-- For Vector512, we only have a very small pool of machines with acceleration support, so they are always outerloop -->

<MergedWrapperProjectReference Include="*/**/*_ro.csproj" Exclude="**/*.RefOnly.csproj" />
<MergedWrapperProjectReference Include="*/**/*_ro.csproj" />

<MergedWrapperProjectReference Remove="Arm/**/*_ro.csproj" Condition="'$(_IncludeArm64HWIntrinsicTests)' != 'true'" />
<MergedWrapperProjectReference Remove="General/Vector64/**/*_ro.csproj" Condition="'$(_IncludeArm64HWIntrinsicTests)' != 'true'" />
Expand Down
2 changes: 0 additions & 2 deletions src/tests/JIT/HardwareIntrinsics/X86/Shared/ScalarTernOpTupleBinRetTest.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
* changes, please update the corresponding template and run according to the *
* directions listed in the file. *
******************************************************************************/
extern alias CoreLib;
using X86Base = CoreLib::System.Runtime.Intrinsics.X86.X86Base;
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
Expand Down
1 change: 0 additions & 1 deletion src/tests/JIT/HardwareIntrinsics/X86/X86Base.X64/X86Base.X64_r.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,5 @@
<ItemGroup>
<Compile Include="Program.X86Base.X64.cs" />
<Compile Include="..\Shared\Program.cs" />
<ProjectReference Include="..\X86Base\DivRem.RefOnly.csproj" Aliases="CoreLib" OutputItemType="ReferencePath" ReferenceOutputAssembly="false" />
</ItemGroup>
</Project>
1 change: 0 additions & 1 deletion src/tests/JIT/HardwareIntrinsics/X86/X86Base.X64/X86Base.X64_ro.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,5 @@
<ItemGroup>
<Compile Include="Program.X86Base.X64.cs" />
<Compile Include="..\Shared\Program.cs" />
<ProjectReference Include="..\X86Base\DivRem.RefOnly.csproj" Aliases="CoreLib" OutputItemType="ReferencePath" ReferenceOutputAssembly="false" />
</ItemGroup>
</Project>
22 changes: 0 additions & 22 deletions src/tests/JIT/HardwareIntrinsics/X86/X86Base/DivRem.RefOnly.csproj
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion src/tests/JIT/HardwareIntrinsics/X86/X86Base/X86Base_r.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,5 @@
<ItemGroup>
<Compile Include="Program.X86Base.cs" />
<Compile Include="..\Shared\Program.cs" />
<ProjectReference Include="DivRem.RefOnly.csproj" Aliases="CoreLib" OutputItemType="ReferencePath" ReferenceOutputAssembly="false" />
</ItemGroup>
</Project>
1 change: 0 additions & 1 deletion src/tests/JIT/HardwareIntrinsics/X86/X86Base/X86Base_ro.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,5 @@
<ItemGroup>
<Compile Include="Program.X86Base.cs" />
<Compile Include="..\Shared\Program.cs" />
<ProjectReference Include="DivRem.RefOnly.csproj" Aliases="CoreLib" OutputItemType="ReferencePath" ReferenceOutputAssembly="false" />
</ItemGroup>
</Project>