Skip to content

Implement MethodImplOptions.AggressiveInlining flag - fixes #1637 #3154

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.

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

Merged
merged 9 commits into from
Jun 26, 2017

Conversation

forki
Copy link
Contributor

@forki forki commented May 30, 2017

@forki
Copy link
Contributor Author

forki commented May 30, 2017

@dsyme @KevinRansom @kbattocchi I tried to find all places that were touched by NoInline but it's not working yet. Any ideas?

dsyme
dsyme previously requested changes Jun 16, 2017
Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

I put some comments above. I think we don't need to change the TAST - just use Never as mentioned above in the case where the attribute is present on a method/function

@@ -123,6 +125,7 @@ type ValFlags(flags:int64) =
(match inlineInfo with
| ValInline.PseudoVal -> 0b0000000000000000000L
| ValInline.Always -> 0b0000000000000010000L
| ValInline.Aggressive -> 0b0100L
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not right - 0b0100L is already used by NormalVal above

@@ -62,6 +62,8 @@ type ValInline =
| PseudoVal
/// Indicates the value is inlined but the .NET IL code for the function still exists, e.g. to satisfy interfaces on objects, but that it is also always inlined
| Always
/// Indicates the value will be inlined be inlined by the .NET runtime
| Aggressive
Copy link
Contributor

@dsyme dsyme Jun 16, 2017

Choose a reason for hiding this comment

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

I'm not convinced we need to change the TAST (or Pickle) at all?

@dsyme dsyme changed the title [WIP] Implement MethodImplOptions.AggressiveInlining flag - fixes #1637 Implement MethodImplOptions.AggressiveInlining flag - fixes #1637 Jun 23, 2017
@dsyme
Copy link
Contributor

dsyme commented Jun 23, 2017

@forki This is now ready but needs a test. Adding a code generation baseline test would make sense. Would you like to do that? thanks!

@forki
Copy link
Contributor Author

forki commented Jun 23, 2017 via email

@dsyme
Copy link
Contributor

dsyme commented Jun 23, 2017

Sorry in foreseeable future I won't have time to work on it.

Ok, I can add this one

@dsyme dsyme dismissed their stale review June 23, 2017 15:49

done

@dsyme
Copy link
Contributor

dsyme commented Jun 23, 2017

OK, test added, this is ready once green

@dsyme dsyme merged commit 3ff08d4 into dotnet:master Jun 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants