Update intrinsics doc based on recent discussions by CarolEidt · Pull Request #28 · dotnet/designs

CarolEidt · 2018-01-13T02:04:47Z

No description provided.

CarolEidt · 2018-01-13T02:11:19Z

@eerhardt @fiigii @sdmaclea @tannergooding @mikedn @4creators @terrajobst @russellhadley
@dotnet/jit-contrib

Also, please ping anyone else I should have included.

tannergooding · 2018-01-13T02:31:24Z

+1. A platform intrinsic should expose a specific feature or semantic that is not universally available on all platforms, and for which the implemented function is not easily recognizable by the JIT.  
   * If the functionality is common and performant - make it platform
-   independent. 
+   independent.


I think I commented on this during the initial design as well, but it might be worth extending this section here.

There is some functionality that is common and performant but which, when exposed in a platform independent manner, loses some of the flexibility that makes it not suitable for use in all cases.

I think a good number of the vector intrinsics fall into this category. They are generally available on all architectures we support, are performant, and we exposed general functionality via the System.Numerics.Vector APIs. However, they lost some of their flexibility, required more complex (custom) support in the JIT, etc.

Additionally, even in the case where we know a general purpose API should be exposed for certain functionality and that functionality is generally supported on modern hardware (CRC32, BitOps, etc) there will need to be custom JIT support for the "fast-path". Exposing that custom support in the form of an intrinsic, and then having the general purpose API call the intrinsic, IMO, is the right way to do such APIs as it provides a general purpose API, but also gives direct access for users who may want or require it.

@tannergooding 's suggestion is consistent with the push back I got for a common crc32 API.

tannergooding · 2018-01-13T02:38:10Z

+- On Intel architecture, there are intrinsics that are only available on float vectors in earlier classes, but are offered for a broader range of types in later classes (e.g. Add over 256-bit vectors on AVX vs. AVX2). For these, it makes sense to explicitly declare the supported types.
+- ARM64 intrinsics include comparisons against zero, but they do not support unsigned types. For these, a trivial JIT expansion could be used to enable all the primitive types to be supported.
+
+In general, it is preferable for a particular instantiated type to not be visible in the API, rather than for the use of an unsupported type to result in a runtime error.


The wording on the sentence is a little confusing.

tannergooding · 2018-01-13T02:41:15Z

+      supported for the current architecture. See [Supporting Generic Intrinsics](#suporting-generic-intrinsics) above.
+   * If the intrinsic fails to meet the necessary criteria:
+      * If the method is recursive, i.e. `gtIsRecursiveCall()` returns true:
+         * If the constraint is one that the JIT must expand in order to support reflection and diagnostic tools (currently only the immediate operand case), it will produce an expanded implementation (e.g. a switchtable of possible immediate values).


Is it worthwhile explicitly calling out that a software implementation is undesirable (that is, an implementation that emulates the functionality, rather than a compiler fallback which still ends up executing the target instruction with the given inputs, such as via a switchtable), since it may behave differently/etc?

tannergooding · 2018-01-13T02:44:15Z


 Q: Is a software fallback implementation allowed? (Discussed above a bit)  
-A: Fallback is allowed but not required.  For very low level
+A: Fallback is allowed but not required, except in the immediate operand as described above.  For very low level


Is there any reason a fallback should be allowed, I can't think of any cases (outside an immediate operand) where some other fallback is needed. All other cases should be able to emit the instruction (even if that is in the recursive call).

I am with @tannergooding on not allowing software fallback except when required to support tools or reflection

Also immediate case is a JIT/codegen fallback, not a software fallback. The original doc was talking about a C# fallback.

tannergooding · 2018-01-13T02:45:23Z

-A: We're planning to add support for this through Roslyn but haven't 
-settled on an implementation yet.  Stay tuned.
+Q: If C# adds support for a const qualifier, will this eliminate the need for switchtable expansion in the JIT?  
+A: Probably not. While it would improve the usability.


Even with C# support, there are other compilers that may not recognize the attribute and indirect calls would still end up with a non-constant value.

Perhaps it would be a good place to inform about the plans to use analyzers for enforcing constant parameters

tannergooding

Overall, LGTM

sdmaclea · 2018-01-15T15:05:48Z

+## Versioning and Partial Implementation
+
+The platform intrinsics will be versioned with the runtime. New intrinsics can be added within an existing platform class, but all intrinsics declared in the platform class must be implemented in the JIT at the same time that it is exposed in the library.
+


Target Framework was the term @eerhardt recommended.

sdmaclea

Add comments regarding lack of compile time type safety.
Add comments regarding PlatformNotSupportedException, TypeNotSupportedException, and ArgumentRangeException being typical/exhaustive set of exceptions
Add comments regarding use of analyzers.
Add comments regarding expectation that code using intrinsics will be carefully scrutinized/optimized/profiled by implementors

At high-level intrinsics are methods of
static classes that are marked with the [Intrinsic] attribute.

We are currently using the namespace and/or a recursive implementation in CoreCLR.

throw new NotImplementedException();

We do not use this in any on the C# implementations.

CoreFX reference uses throw null;
CoreCLR uses recursive call
OR CoreCLR uses platform not supported exception.

Note: This example hasn't been implemented yet - thus NotImplementedException
rather than PlatformNotSupportedException. So details could change going
forward.

This could be edited

sdmaclea · 2018-01-15T15:07:49Z

+1. A platform intrinsic should expose a specific feature or semantic that is not universally available on all platforms, and for which the implemented function is not easily recognizable by the JIT.  
   * If the functionality is common and performant - make it platform
-   independent. 
+   independent.


@tannergooding 's suggestion is consistent with the push back I got for a common crc32 API.

sdmaclea · 2018-01-15T15:08:26Z

 user problem.  Stated another way, platform intrinsics add complexity to the 
 runtime implementation and language stack so they should help a concrete user 
 scenario.
+   * If a set of intrinsics are logically associated (e.g. a vector intrinsic that operates over multiple base types), those intrinsics should generally be implemented together, even if only a subset have compelling impact.


Time permitting

4creators · 2018-01-15T21:04:46Z

@@ -14,30 +14,30 @@ consistent, imposes a performance penalty unacceptable to implementors seeking
 maximum app/service throughput.  To enable this last bit of performance


nit: in original, not modified text, Intel is sometimes used without first uppercase letter

eerhardt · 2018-01-16T16:38:31Z

    public static bool IsSupported() { throw new NotImplementedException(); }

I know this was existing, and not part of your change. But can you fix this to be a property public static bool IsSupported { get { } }, to align with the text and the code?

Refers to: accepted/platform-intrinsics.md:54 in 9606675. [](commit_id = 9606675, deletion_comment = False)

eerhardt · 2018-02-26T20:50:27Z

@CarolEidt - can we get the edits merged sometime soon?

CarolEidt · 2018-03-21T14:47:52Z

@eerhardt @sdmaclea @4creators @fiigii - I believe that I have incorporated all of the feedback, and have merged the edits.

Update intrinsics doc based on recent discussions

9606675

tannergooding reviewed Jan 13, 2018

View reviewed changes

tannergooding approved these changes Jan 13, 2018

View reviewed changes

sdmaclea reviewed Jan 15, 2018

View reviewed changes

4creators reviewed Jan 15, 2018

View reviewed changes

CarolEidt mentioned this pull request Jan 18, 2018

[Arm64] Initial HW intrinsic framework dotnet/coreclr#15833

Merged

CarolEidt added 2 commits March 20, 2018 18:49

Update platform-intrinsics.md

f86b671

Update platform-intrinsics.md

b44acb3

CarolEidt merged commit f316a4a into dotnet:master Mar 21, 2018

CarolEidt deleted the UpdateIntrinsicsDoc branch March 21, 2018 14:46

		## Versioning and Partial Implementation

		The platform intrinsics will be versioned with the runtime. New intrinsics can be added within an existing platform class, but all intrinsics declared in the platform class must be implemented in the JIT at the same time that it is exposed in the library.

		@@ -14,30 +14,30 @@ consistent, imposes a performance penalty unacceptable to implementors seeking
		maximum app/service throughput. To enable this last bit of performance

Conversation

CarolEidt commented Jan 13, 2018

Uh oh!

CarolEidt commented Jan 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4creators Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4creators Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Jan 16, 2018

Uh oh!

eerhardt commented Feb 26, 2018

Uh oh!

CarolEidt commented Mar 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

4creators Jan 15, 2018 •

edited

Loading

4creators Jan 15, 2018 •

edited

Loading