Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 22, 2025

Summary

This PR adds generic versions of the And and Or methods to System.Threading.Interlocked as specified in API proposal #114568.

Changes

  • Add generic And<T>(ref T, T) where T : struct method
  • Add generic Or<T>(ref T, T) where T : struct method
  • Support integer primitive types and enums backed by integer types
  • Reject float/double types and floating-point backed enums
  • Add appropriate error handling and messages
  • Update reference assembly
  • Add comprehensive theory-based tests (40+ test methods with 100+ test cases)
  • Replace existing usages of Interlocked.And/Or(ref Unsafe.As...) with new generic API
  • Restrict JIT intrinsic expansion to TYP_INT and TYP_LONG only
  • Add TODO comment for small integer intrinsic support
  • Update XML documentation to clarify floating-point restrictions
  • Simplify validation logic to single-level if statement
  • Security checks (CodeQL - no issues)

Implementation Details

  • For 1-2 byte types (byte, sbyte, short, ushort): Uses CompareExchange-based loop (managed fallback)
  • For 4-byte types (int, uint): Delegates to int And/Or with bitcast (JIT intrinsic)
  • For 8-byte types (long, ulong): Delegates to long And/Or with bitcast (JIT intrinsic)
  • Float/Double: Rejected via type checks
  • Floating-point backed enums: Rejected via Type.GetEnumUnderlyingType() check (JIT-foldable)
  • JIT Integration: Modified to only expand intrinsic for TYP_INT and TYP_LONG; other types fall back to managed
  • Pattern: Follows same approach as generic CompareExchange<T>

JIT Changes

Modified importercalls.cpp to restrict intrinsic expansion:

  • Only TYP_INT and TYP_LONG are expanded as atomic operations
  • All other types (byte, short, float, double) fall back to managed CompareExchange loops
  • Uses callType consistently for type checks
  • Added TODO comment: "Implement support for XAND/XORR with small integer types (byte/short)"
  • Ensures correct behavior across all supported primitive types and enums

Type Rejection

The methods reject:

  • Non-primitive, non-enum types
  • Floating-point primitives (float, double)
  • Enums backed by floating-point types (via Type.GetEnumUnderlyingType())

The validation check is a single-level if statement with proper indentation:

if ((!typeof(T).IsPrimitive && !typeof(T).IsEnum) ||
    typeof(T) == typeof(float) || typeof(T) == typeof(double) ||
    (typeof(T).IsEnum && (typeof(T).GetEnumUnderlyingType() == typeof(float) || typeof(T).GetEnumUnderlyingType() == typeof(double))))
{
    throw new NotSupportedException(SR.NotSupported_IntegerEnumOrPrimitiveTypeRequired);
}

Documentation

The XML documentation clearly states the restrictions:

/// <typeparam name="T">
/// The type to be used for <paramref name="location1"/> and <paramref name="value"/>.
/// This type must be an integer primitive type or an enum type backed by an integer type.
/// Floating-point types (float, double) are not supported.
/// </typeparam>

Test Coverage

Converted all tests to [Theory] with [InlineData] covering:

  • Multiple test cases per type: 5-10 values each including edge cases
  • Negative values: Comprehensive coverage for all signed types (sbyte, short, int, long)
  • Edge cases: Min/max values, zero, all-ones, alternating bit patterns
  • Float/Double/Half NotSupportedException tests: Verify rejection of floating-point types
  • Null reference exceptions: Separate tests for all types
  • Unsupported types: Tests for non-primitive value types

Usages Replaced

Replaced 7 call sites across the codebase:

  • System.Private.CoreLib/ComWrappers.cs (2 calls)
  • System.Net.Http/CacheControlHeaderValue.cs (2 calls)
  • System.Private.Uri/Uri.cs (1 call)
  • System.Private.Uri/UriSyntax.cs (1 call)
  • System.Private.Uri/UriScheme.cs (1 call)

All usages now use the cleaner generic API without Unsafe.As casts.

Test Results

✅ All 705 System.Threading tests pass (0 errors, 0 failures)

Security

✅ CodeQL analysis passed with no issues

Original prompt

We have the following public APIs in System.Threading.Interlocked class:

public static int Or(ref int location1, int value)
public static uint Or(ref uint location1, uint value)
public static long Or(ref long location1, long value)
public static ulong Or(ref ulong location1, ulong value)

public static int And(ref int location1, int value)
public static uint And(ref uint location1, uint value)
public static long And(ref long location1, long value)
public static ulong And(ref ulong location1, ulong value)

We want to add their generic versions:

public static T Or<T>(ref T location1, T value) where T : struct;
public static T And<T>(ref T location1, T value) where T : struct;

for the same types + enums backed by those types.

Similar to what we did for CompareExchange in #104558.
NOTE: this will require a bit of JIT work, e.g. NI_System_Threading_Interlocked_Or and NI_System_Threading_Interlocked_And

Create a PR in dotnet/runtime to add that. Effectively, implement this API proposal: #114568


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits October 22, 2025 17:51
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
…methods

Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Copilot AI changed the title [WIP] Add generic versions of Or and And methods in Interlocked class Add generic And<T> and Or<T> methods to System.Threading.Interlocked Oct 22, 2025
Copilot AI requested a review from EgorBo October 22, 2025 18:15
Copilot finished work on behalf of EgorBo October 22, 2025 18:15
@EgorBo
Copy link
Member

EgorBo commented Oct 22, 2025

@copilot find existing usages of Interlocked.And(ref Unsafe.As and Interlocked.Or(ref Unsafe.As and replace them with the new API.

Found and replaced all usages of `Interlocked.And(ref Unsafe.As<EnumType, IntType>(ref field), value)`
and `Interlocked.Or(ref Unsafe.As<EnumType, IntType>(ref field), value)` with the new generic API
`Interlocked.And(ref field, value)` and `Interlocked.Or(ref field, value)`.

Affected files:
- System.Private.CoreLib/ComWrappers.cs (CreateComInterfaceFlagsEx enum)
- System.Net.Http/CacheControlHeaderValue.cs (Flags enum)
- System.Private.Uri/Uri.cs (Flags enum)
- System.Private.Uri/UriSyntax.cs (Uri.Flags enum)
- System.Private.Uri/UriScheme.cs (Uri.Flags enum)

Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 22, 2025

@copilot find existing usages of Interlocked.And(ref Unsafe.As and Interlocked.Or(ref Unsafe.As and replace them with the new API.

Done! Found and replaced 7 usages across 5 files:

System.Private.CoreLib

  • ComWrappers.cs: 2 calls (SetFlag/ResetFlag methods with CreateComInterfaceFlagsEx enum)

System.Net.Http

  • CacheControlHeaderValue.cs: 2 calls (SetFlag method with Flags enum)

System.Private.Uri

  • Uri.cs: 1 call (SetFlags method)
  • UriSyntax.cs: 1 call (InternalValidate method)
  • UriScheme.cs: 1 call (InitializeAndValidate method)

All calls now use Interlocked.And/Or(ref enumField, enumValue) directly without Unsafe.As casts. Libraries build successfully. (2334be3)

Copilot finished work on behalf of EgorBo October 22, 2025 19:31
@EgorBo
Copy link
Member

EgorBo commented Oct 22, 2025

@copilot in importercallls.cpp for:

            case NI_System_Threading_Interlocked_Or:
            case NI_System_Threading_Interlocked_And:

can you disable the intrinsic expansion for genActualType(callType) being anything other than TYP_INT and TYP_LONG? it should fallback to managed instead.

Modified importercalls.cpp to only expand the intrinsic for And/Or operations when
genActualType(callType) is TYP_INT or TYP_LONG. Other types (like byte, short) now
fall back to the managed implementation which uses CompareExchange loops.

This ensures that:
- int/uint and long/ulong use native atomic instructions
- byte/sbyte and short/ushort use managed CompareExchange-based implementation
- Generic method calls work correctly for all supported types

Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 22, 2025

@copilot in importercallls.cpp for:

            case NI_System_Threading_Interlocked_Or:

...

Done! Modified importercalls.cpp to only expand the intrinsic when genActualType(callType) is TYP_INT or TYP_LONG. Other types now fall back to the managed CompareExchange-based implementation.

The change:

  • Extracts actualType once at the beginning
  • Adds check: if ((actualType != TYP_INT) && (actualType != TYP_LONG)) { break; }
  • Keeps existing x86 TYP_LONG restriction before the new check
  • Uses actualType in gtNewAtomicNode call

All 611 System.Threading tests pass. (256f950)

Copilot finished work on behalf of EgorBo October 22, 2025 21:00
@EgorBo
Copy link
Member

EgorBo commented Oct 23, 2025

@copilot Change the added tests:

  1. Add tests for float & double
  2. Add tests for negative values
  3. Convert tests into [Theory] with [InlineData] so they use 5-10 test values & expected results

- Use callType directly in JIT intrinsic check as suggested by @EgorBo
- Allow float/double support (they work via bitcast to int/long for bitwise operations)
- Convert all And/Or generic tests to [Theory] with [InlineData]
- Add 5-10 test cases per type including negative values and edge cases
- Add comprehensive tests for float/double types
- Total test count increased from 611 to 712

All tests pass successfully.

Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 23, 2025

@copilot Change the added tests:

  1. Add tests for float & double
  2. Add tests for negative values
    ...

Done! All test changes implemented:

  1. Float & double tests added: Tests now verify bitwise operations work on float/double (via bitcast to int/long)
  2. Negative values: Added test cases with negative values for all signed types (sbyte, short, int, long)
  3. Theory conversion: All tests converted to [Theory] with [InlineData]:
    • 5-10 test cases per type
    • Covers edge cases (min/max, zero, all-ones, alternating patterns)
    • Includes negative values for signed types
    • Separate tests for null reference exceptions

Test count increased from 611 to 712. All tests pass. (d549cdd)

Copilot finished work on behalf of EgorBo October 24, 2025 11:00
@EgorBo
Copy link
Member

EgorBo commented Oct 24, 2025

@jkotas @stephentoub does this look good? I've used TypeCode to simplify enum check, without it was:

if ((!typeof(T).IsPrimitive && !typeof(T).IsEnum) ||
    typeof(T) == typeof(float) ||
    typeof(T) == typeof(double) ||
    (typeof(T).IsEnum && (typeof(T).GetEnumUnderlyingType() == typeof(float) ||
                            typeof(T).GetEnumUnderlyingType() == typeof(double))))
{
    throw new NotSupportedException();
}

@jkotas
Copy link
Member

jkotas commented Oct 24, 2025

used TypeCode

What does that do to inlining budgets, etc.?

@EgorBo
Copy link
Member

EgorBo commented Oct 24, 2025

used TypeCode

What does that do to inlining budgets, etc.?

Ah, that is a good question, it looks like Type.GetTypeCode(typeof(T)) is not a direct intrinsic and relies on it being inlined, so probably it's not a good choice indeed, didn't know that 🤔

@jkotas
Copy link
Member

jkotas commented Oct 24, 2025

Right. Also, Type.GetTypeCode is legacy and has a dependency on goodies like DbNull. It does not sound right for primitive interlocked operations to have (indirect) dependency on DbNull.

@EgorBo
Copy link
Member

EgorBo commented Oct 24, 2025

Right. Also, Type.GetTypeCode is legacy and has a dependency on goodies like DbNull. It does not sound right for primitive interlocked operations to have (indirect) dependency on DbNull.

I've reverted to GetEnumUnderlyingType. Does it look good otherwise?

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM!

@EgorBo
Copy link
Member

EgorBo commented Oct 26, 2025

@copilot address the feedback over the NotSupportedException check to make it single level.

Changed the NotSupportedException validation logic from nested if statements
to a single-level if with a compound condition using logical operators.

This addresses PR review feedback to:
- Remove unnecessary nesting
- Fix indentation issues
- Make the logic more readable and maintainable

The condition now checks all invalid type scenarios in one expression:
- Non-primitive and non-enum types
- float and double primitives
- Enums backed by float or double

All 705 tests pass successfully.

Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 26, 2025

@copilot address the feedback over the NotSupportedException check to make it single level.

Done. Simplified the validation check to a single-level if statement with a compound condition. Fixed indentation and removed unnecessary nesting. (a1edac8)

Copilot AI requested a review from EgorBo October 26, 2025 13:46
Copilot finished work on behalf of EgorBo October 26, 2025 13:46
@EgorBo EgorBo enabled auto-merge (squash) October 26, 2025 16:44
@EgorBo
Copy link
Member

EgorBo commented Oct 26, 2025

/ba-g deadletter

@EgorBo EgorBo disabled auto-merge October 27, 2025 11:54
@EgorBo EgorBo enabled auto-merge (squash) October 27, 2025 11:54
@EgorBo
Copy link
Member

EgorBo commented Oct 27, 2025

/ba-g deadletter

@EgorBo EgorBo merged commit 2234a3b into main Oct 27, 2025
153 of 157 checks passed
@EgorBo EgorBo deleted the copilot/add-generic-interlocked-methods branch October 27, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants