Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Inliner: enable inlining of methods with conditional throws#8038

Merged
AndyAyersMS merged 2 commits intodotnet:masterfrom
AndyAyersMS:InlineConditionalThrows
Nov 9, 2016
Merged

Inliner: enable inlining of methods with conditional throws#8038
AndyAyersMS merged 2 commits intodotnet:masterfrom
AndyAyersMS:InlineConditionalThrows

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

@AndyAyersMS AndyAyersMS commented Nov 8, 2016

Remove inlining limitation for methods that return values and have
conditional throws. This limitation was most likely a vestige of an
older inlining implementation that did not break trees at inline
call sites.

Also removed the now-unused observation and the seenConditionalJump
member variable. Merged ifdef blocks in impInit.

Ran full desktop testing, no issues.

Enables a handful of inlines in the various code size suites. For the
most part these slightly increase code size but can often shorten the
non-EH paths in the code.

@AndyAyersMS
Inliner: enable inlining of methods with conditional throws
e342633 
Remove inlining limitation for methods that return values and have
conditional throws. This limitation was most likely a vestige of an
older inlining implementation that did not break trees at inline
call sites.

Also removed the now-unused observation and the `seenConditionalJump`
member variable. Merged ifdef blocks in `impInit`.

Ran full desktop testing, no issues.

Enables a handful of inlines in the various code size suites. For the
most part these slightly increase code size but can often shorten the
non-EH paths in the code.
@AndyAyersMS
Copy link
Copy Markdown
Member Author

AndyAyersMS commented Nov 8, 2016

cc @dotnet/jit-contrib

Output from jit-diff:

Details 
 bytes of diff: 1381 (0.02 % of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
         994 : System.Private.CoreLib.dasm (0.03 % of base)
         330 : System.Net.Sockets.dasm (0.19 % of base)
          57 : System.Runtime.Extensions.dasm (0.07 % of base)
3 total files with size differences.
Top method regessions by size (bytes):
         191 : System.Private.CoreLib.dasm - mincore:GetMessage(long,int):ref
         164 : System.Private.CoreLib.dasm - mincore:TryGetErrorMessage(long,int,ref,byref):bool
         154 : System.Private.CoreLib.dasm - System.Guid:TryParseGuidWithNoStyle(ref,byref):bool
         150 : System.Private.CoreLib.dasm - System.Globalization.DateTimeFormatInfo:InsertAtCurrentHashNode(ref,ref,char,int,int,int,int,int):this
         132 : System.Private.CoreLib.dasm - System.Threading.ManualResetEventSlim:Wait(int,struct):bool:this
13 total methods with size differences.

Example code diff:

Details 
;;; Assembly listing for method System.AppDomain:ExecuteAssembly(ref):int:this
;;; Before

G_M56995_IG01:
       sub      rsp, 40
       nop      
G_M56995_IG02:
       xor      r8d, r8d
       mov      dword ptr [rsp+20H], r8d
       xor      r8, r8
       xor      r9, r9
       call     [System.AppDomain:ExecuteAssembly(ref,ref,ref,int):int:this]
       nop      
G_M56995_IG03:
       add      rsp, 40
       ret  

;;; After

G_M56995_IG01:
       push     rsi
       sub      rsp, 32
G_M56995_IG02:
       test     rdx, rdx
       je       SHORT G_M56995_IG05
G_M56995_IG03:
       xor      eax, eax
G_M56995_IG04:
       add      rsp, 32
       pop      rsi
       ret      
G_M56995_IG05:
       call     [CORINFO_HELP_READYTORUN_NEW]
       mov      rsi, rax
       mov      ecx, 425
       call     CORINFO_HELP_STRCNS_CURRENT_MODULE
       mov      rdx, rax
       mov      rcx, rsi
       call     [System.ArgumentNullException:.ctor(ref):this]
       mov      rcx, rsi
       call     CORINFO_HELP_THROW
       int3

@omariom
Copy link
Copy Markdown

omariom commented Nov 8, 2016

So now methods like the following will be inlined?

private int GetLength(string str)
{
   if (str == null)
      throw new ArgumentNullException();

   return str.Length:
}

@AndyAyersMS
Copy link
Copy Markdown
Member Author

AndyAyersMS commented Nov 8, 2016

@omariom the conditional throw will no longer block inlining, so a simple method like this should get inlined.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

AndyAyersMS commented Nov 9, 2016

@dotnet/jit-contrib anyone up for a review? It is a pretty small change...

@erozenfeld
Copy link
Copy Markdown
Member

erozenfeld commented Nov 9, 2016

LGTM

@AndyAyersMS
Copy link
Copy Markdown
Member Author

AndyAyersMS commented Nov 9, 2016

Ignoring the jit diff failure.

@AndyAyersMS AndyAyersMS merged commit e76da56 into dotnet:master Nov 9, 2016
@AndyAyersMS AndyAyersMS deleted the InlineConditionalThrows branch November 9, 2016 23:03
@omariom omariom mentioned this pull request Nov 11, 2016
Merged
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
@JosephTremoulet JosephTremoulet mentioned this pull request Oct 30, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@CarolEidt CarolEidt CarolEidt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@AndyAyersMS @omariom @erozenfeld @CarolEidt @karelz @dnfclas