Public API Proposal: Make bool RuntimeHelpers.IsKnownConstant(...) public for all primitive types

gerhard17 commented 2 years ago

Background and motivation

Recently two internal IsKnownConstant() overloads in class RuntimeHelpers for char and string? were added. For a library developer it would be usefull to consume this API like the framework internaly does.

Same performance/codegen reasons apply as with the original (internal) proposal. #11484

At least one overload for each primitive type (+ the existing string) would be wellcomed.

API Proposal

namespace System.Runtime.CompilerServices
{
    public static partial class RuntimeHelpers
    {
        // The following intrinsics return true if input is a compile-time constant
        // Feel free to add more overloads on demand

       // CHANGE VISIBILITY

        [Intrinsic]
        public static bool IsKnownConstant(string? t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(char t) => false;

        // ADD OVERLOADS

        [Intrinsic]
        public static bool IsKnownConstant(byte t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(sbyte t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(short t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(ushort t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(int t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(uint t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(long t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(ulong t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(nint t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(nuint t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(float t) => false;

        [Intrinsic]
        public static bool IsKnownConstant(double t) => false;

    }
}

API Usage

API can be public used like the current internal usage inside of the runtime.

Alternative Designs

Maybe a true generic Version can be used.

namespace System.Runtime.CompilerServices
{
    public static partial class RuntimeHelpers
    {
        [Intrinsic]
        public static bool IsKnownConstant<T>(T t) => false;
    }
}

Risks

No breaking change. Not an public API today.
Will depend on JIT beyond c# language spec.

dotnet-issue-labeler[bot] commented 2 years ago

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.

teo-tsirpanis commented 2 years ago

I'm in favor of making it generic.

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices See info in area-owners.md if you want to be subscribed.

Issue Details

### Background and motivation Recently two internal IsKnownConstant() overloads in class RuntimeHelpers for char and string? were added. For a library developer it would be usefull to consume this API like the framework internaly does. Same performance/codegen reasons apply as with the original (internal) proposal. #11484 At least one overload for each primitive type (+ the existing string) would be wellcomed. See also: #63734 and #64809 This request has similarities with GNU GCC int __builtin_constant_p (exp) ### API Proposal ```C# namespace System.Runtime.CompilerServices { public static partial class RuntimeHelpers { // The following intrinsics return true if input is a compile-time constant // Feel free to add more overloads on demand // CHANGE VISIBILITY [Intrinsic] public static bool IsKnownConstant(string? t) => false; [Intrinsic] public static bool IsKnownConstant(char t) => false; // ADD OVERLOADS [Intrinsic] public static bool IsKnownConstant(byte t) => false; [Intrinsic] public static bool IsKnownConstant(sbyte t) => false; [Intrinsic] public static bool IsKnownConstant(short t) => false; [Intrinsic] public static bool IsKnownConstant(ushort t) => false; [Intrinsic] public static bool IsKnownConstant(int t) => false; [Intrinsic] public static bool IsKnownConstant(uint t) => false; [Intrinsic] public static bool IsKnownConstant(long t) => false; [Intrinsic] public static bool IsKnownConstant(ulong t) => false; [Intrinsic] public static bool IsKnownConstant(nint t) => false; [Intrinsic] public static bool IsKnownConstant(nuint t) => false; [Intrinsic] public static bool IsKnownConstant(float t) => false; [Intrinsic] public static bool IsKnownConstant(double t) => false; } } ``` ### API Usage API can be public used like the current internal usage inside of the runtime. ### Alternative Designs Maybe a true generic Version can be used. ```C# namespace System.Runtime.CompilerServices { public static partial class RuntimeHelpers { [Intrinsic] public static bool IsKnownConstant(T t) => false; } } ``` ### Risks - No breaking change. Not an public API today. - Will depend on JIT beyond c# language spec.

Author:	gerhard17
Assignees:	-
Labels:	`api-suggestion`, `area-System.Runtime.CompilerServices`, `untriaged`
Milestone:	-

SupinePandora43 commented 2 years ago

I'm in favor of making it generic.

Will it be able to consume null reference types and default value types?

EgorBo commented 2 years ago

1) The generic version must be guarded with : unmanaged constraint because with ref types it might introduce a runtime lookup that will prevent inlining 2) Is __builtin_constant popular in the C/C++ world? I've never seen it in the wild. Do you have a real-world scenario where it'd help you? 3) There are unresolved concerns with this API and JIT's inliner, e.g.:

void DoWork(int a)
{
    if (RuntimeHelpers.IsKnownConstant(a) && a == 42)
    {
        // a lot of code
    }
    // a lot of code
}

jit (currently) is not able to give it additional inlining boost only when a == 42 (currently it doesn't boost at all)

gerhard17 commented 2 years ago

@EgorBo

I would be fine with restriction unmanaged.
I found the C/C++ documentation during internet search. I just searched, if the JIT already implements such a thing. I for myself being somehow familiar with C/C++, I never used it in production code. :-)
Yes, currently I have such a case in an arbitrary presision floating point library. Common numeric constants given like literals would greatly benefit from this.
Yes this is similar to my use case. Only a little bit longer switch statement like:

[MethodImpl(MethodImplOptions.AggressiveInlining)]
void DoWork(int a)
{
    if (RuntimeHelpers.IsKnownConstant(a))
    {
        switch(a) {
        case 42:
          // some code
          return;
        case 43:
          // some code
          return;
         // some more cases with some code
       }
    }
    // a lot of generalized, but slower code
}

Btw. thanks for all the cool coding I found by you in this repository!

gerhard17 commented 2 years ago

Could restriction unmanaged be dangerous with large structs? Copying struct data, when no JIT is involved. Contradicting the performance gain in non JIT environments.

SupinePandora43 commented 2 years ago

Could restriction unmanaged be dangerous with large structs? Copying struct data, when no JIT is involved. Contradicting the performance gain in non JIT environments.

What about in and ref readonly?

jkotas commented 2 years ago

unresolved concerns with this API and JIT's inliner

Yep, it would be useful to see this API used in libraries in more places to prove that it actually works except for a few trivial carefully crafted cases.

https://github.com/dotnet/runtime/pull/64821 was unsuccessfully in using this API for more complex code.

buyaa-n commented 2 years ago

Tag @GrabYourPitchforks for triage as he created the internal API proposal

gerhard17 commented 2 years ago

@jkotas How does the annotation [MethodImpl(MethodImplOptions.AggressiveInlining)] change the inlining behavior?

Will all code be inlined or is only the budget increased to inline more (but maybe not all) code?

EgorBo commented 2 years ago

@jkotas How does the annotation [MethodImpl(MethodImplOptions.AggressiveInlining)] change the inlining behavior?

Will all code be inlined or is only the budget increased to inline more (but maybe not all) code?

Yes, large AggressiveInlining calls may eat the inliner budget pretty quickly - that is something we plan to fix eventually - I mean inliner should be smarter and do not take large pieces (branches/blocks) of methods which will be eliminated at the import stage into account, just not sure it will make it to .NET 7.0

Example: sharplab.io

GrabYourPitchforks commented 2 years ago

Tag GrabYourPitchforks for triage as he created the internal API proposal

I agree with Jan's assessment at https://github.com/dotnet/runtime/issues/67285#issuecomment-1081889927. This is experimental and very temperamental. We need more evidence that it's widely useful before exposing it publicly. It honestly wouldn't surprise me if the JIT folks somehow come up with some clever scheme which obviates the utility of IsKnownConstant entirely and we end up removing the API as a result.

redknightlois commented 4 months ago

This code should have a IsKnownConstant guard in the size. Currently we do it by code inspection. https://github.com/ravendb/ravendb/blob/c0bbece37e7b3d3afff2010b6b0e108633fd136e/src/Sparrow/Memory.cs#L472

We have a few examples on our codebase that could use a generic version.

dotnet / runtime