Compiler intrinsics #1685

jaredpar · 2018-07-02T05:18:37Z

This is an alternate proposal for the compiler intrinsicts feature:

This alternate design proposal comes after reviewing a prototype implementation
of the original proposal by @msjabby as well as the use throughout a significant
code base.

This design was done with significant input from @mjsabby, @tmat and @jkotas.

@mjsabby

This is an alternate proposal for the compiler intrinsicts feature: dotnet#191 dotnet/roslyn#11475 This alternate design proposal comes after reviewing a prototype implementation of the original proposal by @mjsabby as well as the use throughout a significant code base. This design was done with significant input from @mjsabby, @tmat and @jkotas.

jkotas · 2018-07-02T05:25:22Z

intrinsics.md

+Restrictions of this feature:
+
+- Properties cannot be used in a `handleof` expression.
+- The `handleof` expression cannot be used when there is an existing `handelof` name in scope. For 


Typo: handelof

nguerrera · 2018-07-02T05:52:07Z

intrinsics.md

+[AttributeUsage(AttributeTargets.Method)]
+public sealed class CallIndirectAttribute : Attribute
+{
+    public CallingConvention CallingConvention { get; set; }


Should be read-only. Design guidelines say that attribute arguments should be either positional or named, not both.

jkotas · 2018-07-02T05:55:26Z

intrinsics.md

+This allows developers to define methods in the following form:
+
+``` csharp
+[CallIndirect(CallingConvention.Cdecl)]


This enum does not have a value for regular managed calling convention. (@mjsabby use case and some other use cases needs the managed calling convention).
I think we may need to have two attributes:

CallIndirectAttribute for regular managed calls that does not have CallingConvention.

UnmanagedCallIindirectAttribute for unmanaged calls that does have CallingConvention

An alternative is to add a value to for the managed calling convention to CallingConvention enum. I have rejected it because of the new value would be invalid in all existing APIs that use CallingConvention enum today. All these APIs are for unmanaged calls.

add a value to for the managed calling convention to CallingConvention enum

Add a new value in CallingConvention seems better than use two attributes. simpler and easy to understand.

@luqunl I agree in principle that two attributes are more annoying - however, I agree with @jkotas that adding it to the enum is less than ideal.

Another approach that would work from the API perspective would be to make the default constructor for CallIndirectAttribute specify a sentinel value of sorts that indicates managed convention - a default that probably makes sense - and another constructor that takes a CallingConvention enum value. In this case the default is 'managed to managed' and if a calling convention is used the intent is to leave the managed world so let us know what is being called.

The nice aspect about this is that the sentinel value can be anything and doesn't require breaking the API boundary.

Let me add an open issues to track this down. The language design can proceed while we figure this out.

jkotas · 2018-07-02T06:16:30Z

intrinsics.md

+- Local functions cannot be used in `&`. The implementation details of these methods are
+deliberately not specified by the language. This includes whether they are static vs. instance or
+exactly what signature they are emitted with.
+


We should productize the NativeCallableAttribute that will work together with this. This proposal together with NativeCallableAttribute will deliver complete solution for low-level managed/unmanaged interop for function pointers:

address-of + NativeCallableAttribute can be used the get unmanager pointer of method implemented in C#

Calli indirect can be used to call unmanaged pointer

.NET Core supports the NativeCallableAttribute attribute internally, but the feature was not exposed in public contracts and documented.

@jeffschwMSFT @AaronRobinsonMSFT

What does this attribute do today in the internal support?

It adds unmanaged->managed->unmanaged transition to the method prolog/epilog.

Marking the method with NativeCallableAttribute and taking its address is more efficient low-level equivalent of Marshal.GetFunctionPointerForDelegate. It avoids the delegate wrapping and unwrapping overhead. More details are in description for dotnet/coreclr#1566

Marking the method with NativeCallableAttribute and taking its address is more efficient low-level equivalent of Marshal.GetFunctionPointerForDelegate

They are not exactly equivalent. The first one doesn't support marshalling(you need to use blittable types), the second one support marshalling.

Chatted with @AaronRobinsonMSFT and I better understand how this could interact with the feature. Agree there are benefits. Seems easy to add if the runtime decides to productize this. Will add this to the "future consideration" section.

tmat · 2018-07-02T06:24:18Z

intrinsics.md

+
+### calli
+
+The compiler will add support for a new type of `extern` function that efficients translates into


Typo: efficients

tmat · 2018-07-02T06:24:34Z

intrinsics.md

+### calli
+
+The compiler will add support for a new type of `extern` function that efficients translates into
+a `.calli` instruction. The exten attribute will be marked with an attribute of the following


Typo: exten

tmat · 2018-07-02T06:26:22Z

intrinsics.md

+
+This was rejected because a compelling case could not be made nor could a simple syntax be 
+envisioned here. Also there is a fairly straight forward work around: simple define another 
+method, possible local function, that is unambiguous and uses C# code to call into the 


This statement:

possible local function

contradicts statement above:

Local functions cannot be used in &. The implementation details of these methods are
deliberately not specified by the language. This includes whether they are static vs. instance or
exactly what signature they are emitted with.

tmat · 2018-07-02T06:27:54Z

intrinsics.md

+
+``` csharp
+unsafe { 
+    static void LocalLog() => Util.Log();


Isn't this an example of a static local function discussed in Future Considerations below?

It is. I was trying to keep the sample concise. I will make it clearer by using a plain old static.

mjsabby · 2018-07-02T07:33:36Z

intrinsics.md

+The addressof expression in this context will be implemented in the following manner:
+
+- ldftn: when the method is static 
+- ldvirtftn: when the method is an instance method.


Would we allow the emission of ldftn if we know that the method is non-virtual?

C# always uses callvirt instead of call and I was trying to mirror that here. I don't have any particular reason beyond that.

Having a way to do both ldvirtftn and ldftn would let library writers determine whether a method has been overridden by a subclass. This has been used in performance optimization in the past, but it's unavailable to code that doesn't live in the runtime.

BTW: There is nothing that says that the runtime has to return the same pointer from ldvirtftn all the time. .NET Framework / Core do actually return different pointers for the same method.

If the pointer equality is used as an optimization, comparing function pointers will work but not reliably.

If the pointer equality is used for correctness, comparing function pointers is not going to work well. (Your HasOverriddenBeginEndRead example is used as both performance optimization but also to achieve correctness.)

There is nothing that says that the runtime has to return the same pointer

We could add public API to compare function pointers for equality, couldn't we? Then this optimization would become available to library developers outside of CoreLib (provided we get a ldvirtftn/ldftn distinction in this proposal).

I'm calling this out because the original proposal in dotnet/roslyn#11475 had this, but this one doesn't. But I'll be happy if this makes it into Roslyn even in the currently proposed form.

@MichalStrehovsky

The difference between these two for non-virtual methods would only be about whether we get a NullReferenceException for a null this, right?

For call and callvirt this also affects binary compatibility. Imagine a method in a utility library changes from non-virtual to virtual and a consuming assembly isn't recompiled. In C# the code in the consuming assembly will automatically begin calling the method as a virtual.

I'm unsure if ldftn and ldvirtftn behave in a similar way. Been trying to track that down but haven't found any good dos on it. Assuming they do then it would be a consideration when deciding what code the address-of & operator generated.

For call and callvirt this also affects binary compatibility

Isn't this just a side effect, as opposed from being a conscious design choice? When the compiler knows the this is going to non-null, it will happily generate a call:

public class Base { public void Frob() { } } class Derived : Base { public void Call() { // Emits call Frob(); } public static void CallStatic(Derived d) { // Emits callvirt d.Frob(); // Emits call d?.Frob(); } }

Isn't this just a side effect, as opposed from being a conscious design choice?

It's hard to say whether in C# 2.0 (where callvirt became the default over call) whether binary compat was a concern. At this point though it's a known quantity of the language that we consider when making changes. It's not an unbreakable change by any means but it's a consideration we make.

I think we're over focusing on the callvirt thing here a bit. It's not a big blocking problem so much as it is a box that needs to be checked. I don't suspect there would be any substantial push back from LDM if we make a different decision for how ldftn / ldvirtftn were chosen.

Alright, LGTM.

jnm2 · 2018-07-02T13:17:09Z

intrinsics.md

+
+## Motivation
+
+The motivations and background for this feature are described in the following isssue (as is a 


Typo isssue

sylveon · 2018-07-02T13:44:25Z

How would the call indirect attribute mix with P/Invoke?

jkotas · 2018-07-02T14:30:51Z

How would the call indirect attribute mix with P/Invoke?

DllImportAttribute and CallIndirectAttribute do not mix. Having both should produce compile error.

jaredpar · 2018-07-02T16:13:00Z

intrinsics.md

+* [x] Proposed
+* [ ] Prototype: Not Started
+* [ ] Implementation: Not Started
+* [ ] Specification: Below


Will move this to the issue only.

tannergooding · 2018-07-02T16:49:12Z

The Static Delegates proposal would be fully emulatable in managed code if this was implemented.

Perhaps it would be worth just implementing them simultaneously (assuming the additional overhead wasn't too great)?

jaredpar · 2018-07-02T17:15:33Z

@tannergooding

static delegates require further runtime work to be done in a safe fashion. There needs to be some way to prevent assembly unloading when a static delegate points inside it. This proposal side steps this problem by forcing all such operations to be unsafe.

tannergooding · 2018-07-02T17:22:46Z

@jaredpar, you could just make static delegates require unsafe in v1 and then relax that when the runtime support gets in 😉 (but it's also no big deal, since I can just modify my binding generator to emulate the functionality for the time being)

AaronRobinsonMSFT · 2018-07-02T17:26:35Z

intrinsics.md

+### Disambiguating method groups
+
+There was some discussion around features that would make it easier to disambiguate method groups
+passed to an addressof expression. For instance potentially adding signature elements to the 


addressof <- should be surrounded by back ticks (`)

It's not code though, it's a language concept.

I think some people use it to highlight keywords as well 😄

(whether language keywords or words that are key to the sentence)

If this is a language concept, then it should be hyphenated "address-of" (i.e. 'addressof' is not a word).

AaronRobinsonMSFT · 2018-07-02T17:47:32Z

intrinsics.md

+### handleof
+
+The `handleof` contextual keyword will translate a field, member or type into their equivalent 
+`RuntimeHandle` using the `ldtoken` instruction.


Just for clarity - I was not familiar with RuntimeHandle - it technically is RuntimeXXXXHandle right? Want to make sure and not assume something new will be created. It might be beneficial to enumerate all of them at least once and then use the RuntimeHandle as the placeholder going forward.

Correct. I will clarify that here.

333fred · 2018-07-02T17:51:16Z

intrinsics.md

+void* ptr = &Util.Log; 
+```
+
+Given there is no delegate conversion here the only mechanism for filtering members in the method


Do we think that this is going to be a significant bar for existing libraries to use this feature, if they've already got a number of method overloads that they want to distinguish between?

Ah, I see you have a section on that below, hadn't gotten there yet.

333fred · 2018-07-02T17:56:16Z

intrinsics.md

+}
+```
+
+This was rejected because a compelling case could not be made nor could a simple syntax be 


Did we consider allowing a "cast"? ie, something like &((Action<string>)Util.Log)?

MichalStrehovsky · 2018-07-02T21:51:34Z

intrinsics.md

+
+``` csharp
+[CallIndirect(CallingConvention.Cdecl)]
+static extern int MapValue(string s, void *ptr);


Is the C# compiler going to emit this method into the metadata, or will it disappear at source compilation time? The runtime typically won't load a type that has an extern method without a DllImport or some other special marking.

What happens if I try to reference this method in a non-call context (try to construct a delegate to this or get a RuntimeHandle of it)?

The current implementation generates method bodies for this reason, and at least for the managed calling convention the calli gets inlined. Or would you like roslyn to prevent the cases you're mentioning and not emit metadata?

I was just wondering if it's going to be actually emitted as extern, or extern was just a spare keyword.

@MichalStrehovsky the extern keyword here indicates the method will not have a body that is supplied by the developer. Instead it will be supplied by the compiler, or the runtime depending on how it's omitted. In this case as @mjsabby indicates we'll be putting the calli implementation in the body here.

This is still being fleshed out a bit and is more compiler centric vs. language centric. Hence I didn't dive too deep into that here.

The Project N compiler only needs the prefix to match, so matching the behavior here. dotnet/csharplang#1685 can't come soon enough. https://github.com/dotnet/corert/blob/7246a7a5f32f44dc12b8a538383a4bbc7cc26d91/src/System.Private.Interop/src/Shared/McgIntrinsics.cs#L59-L67

The Project N compiler only needs the prefix to match, so matching the behavior here. dotnet/csharplang#1685 can't come soon enough.

jeffschwMSFT · 2018-07-10T18:00:42Z

@luqunl

luqunl · 2018-07-10T18:58:18Z

intrinsics.md

+This allows developers to define methods in the following form:
+
+``` csharp
+[CallIndirect(CallingConvention.Cdecl)]


We should dis-allow DllImport method with CallIndirect attribute.

luqunl · 2018-07-10T18:58:31Z

intrinsics.md

+### Allow address of to target methods
+
+Method groups will now be allowed as arguments to an address-of expression. The type of such an 
+expression will be `void*`. 


curious: why do we prefer "void *" instead of "IntPtr"? If we use IntPtr, we don't need to use unsafe part?

This is not a safe operation which is part of the motivation for returning a void* here.

luqunl · 2018-07-10T18:59:45Z

intrinsics.md

+
+``` csharp
+[CallIndirect(CallingConvention.Cdecl)]
+static extern int MapValue(string s, void *ptr);


For calli, Will we support generic method? also named arguments?
such as Int MapValue<T1, T2>(T1 a1, T2 a2, void *p)?

Unsure about supporting a generic method. As long as it's supported in the runtime I think we can make it work fine in the language.

As for named arguments: yes. Likely optional as well.

Generic calli is not supported by the runtime.

I think @luqunl meant support for this:

.assembly foo { } .method static !!0 Calli<TResult>(native int) { ldarg.0 calli !!0 () ret } .method static int32 Test() { ldc.i4 42 ret } .method static int32 Main() { .entrypoint ldftn int32 Test() call !!0 Calli<int32>(native int) ret }

Rather than generating a calli whose signature is generic (something like calli !!0<!!0>()).

This seems to work fine.

Allowing this on generic methods would save quite a bit of typing so I would vote for not blocking this.

I agree that it would nice to allow it - if we do the matching work in CoreCLR and other runtimes to support it.

For managed calling convention, it is fine and works.

For unmanaged calling convention, it is going to hit https://github.com/dotnet/coreclr/issues/14524 in CoreCLR.

Allowing this on generic methods would save quite a bit of typing so I would vote for not blocking this.

Yes. Interop would love to support this. Currently MCG and System.Private.Interop will define/generic a lot calli helper methods, similar to the following

internal static void HasThisCall__9<TArg0>(
object __this,
global::System.IntPtr pfn,
TArg0 arg0)
{
// This method is implemented elsewhere in the toolchain
}

would save quite a bit of typing

I do not think that the extra typing is a problem here. This is a low-level feature where saving typing is not important. Also, this would be typically auto-generated, not typed by humans.

I would look at the potential support for generic unmanaged calli as second phase of this feature. Generic unmanaged calli would make this feature significantly more complex because of it would dependent on a runtime feature that does not exist yet.

I would look at the potential support for generic unmanaged calli as second phase of this feature.

FWIW: expanding this to include generic support at a later date is basically a bug fix level change. I agree a second phase is a good landing place for this.

Generic unmanaged calli would make this feature significantly more complex because of it would dependent on a runtime feature that does not exist yet

It adds complexity yes but not significant complexity to the compiler. The compiler is already setup to look at RuntimeFeatures to enable / disable items we know depend on runtime changes. As long as the fix in CoreClr included an entry here we could depend on it.

That being said I think we can move forward for now with generic disabled and revisit once we make a bit more progress. It won't really need language buy off at that point, just keyboard time to get the change in.

jaredpar · 2018-07-16T21:50:57Z

Updated PR based on feedback.

jaredpar self-assigned this Jul 2, 2018

jaredpar force-pushed the fix-intrinsic branch from ff1e90b to 3b4697d Compare July 2, 2018 05:21

jkotas reviewed Jul 2, 2018

View reviewed changes

jkotas approved these changes Jul 2, 2018

View reviewed changes

nguerrera reviewed Jul 2, 2018

View reviewed changes

jkotas reviewed Jul 2, 2018

View reviewed changes

tmat reviewed Jul 2, 2018

View reviewed changes

mjsabby reviewed Jul 2, 2018

View reviewed changes

jnm2 reviewed Jul 2, 2018

View reviewed changes

jaredpar commented Jul 2, 2018

View reviewed changes

AaronRobinsonMSFT reviewed Jul 2, 2018

View reviewed changes

333fred reviewed Jul 2, 2018

View reviewed changes

MichalStrehovsky reviewed Jul 2, 2018

View reviewed changes

MichalStrehovsky mentioned this pull request Jul 4, 2018

Fix handling of StdCall intrinsic dotnet/corert#6051

Merged

MichalStrehovsky added a commit to dotnet/corert that referenced this pull request Jul 4, 2018

Fix handling of StdCall intrinsic (#6051)

51e9932

The Project N compiler only needs the prefix to match, so matching the behavior here. dotnet/csharplang#1685 can't come soon enough.

luqunl reviewed Jul 10, 2018

View reviewed changes

mjsabby approved these changes Jul 16, 2018

View reviewed changes

jaredpar added 2 commits July 16, 2018 10:54

Respond to PR feedback

7b87ad9

Trigger PR

3bd8afd

jaredpar merged commit 17a3001 into dotnet:master Jul 17, 2018

jaredpar mentioned this pull request Jul 19, 2018

Champion "Static Delegates" #302

Closed

5 tasks


		### calli

		The compiler will add support for a new type of `extern` function that efficients translates into


		## Motivation

		The motivations and background for this feature are described in the following isssue (as is a

Compiler intrinsics #1685

Compiler intrinsics #1685

Conversation

jaredpar commented Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

MichalStrehovsky Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylveon commented Jul 2, 2018

jkotas commented Jul 2, 2018

Choose a reason for hiding this comment

tannergooding commented Jul 2, 2018

jaredpar commented Jul 2, 2018

tannergooding commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffschwMSFT commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Jul 16, 2018

jaredpar commented Jul 2, 2018 •

edited

Loading

jkotas Jul 2, 2018 •

edited

Loading

tmat Jul 2, 2018 •

edited

Loading

jkotas Jul 2, 2018 •

edited

Loading

MichalStrehovsky Jul 3, 2018 •

edited

Loading

MichalStrehovsky Jul 3, 2018 •

edited

Loading

jkotas Jul 11, 2018 •

edited

Loading