Set DOES_NOT_RETURN gtCallMoreFlags in fgFindJumpTargets #6742

benaadams · 2016-08-14T08:13:09Z

Adds gtCallMoreFlags |= GTF_CALL_M_DOES_NOT_RETURN to fgFindJumpTargets which happens in early in pipeline (which may mark the method as uninlinable, so prevent the later setting in fgFindBasicBlocks).

Count the BBJ_RETURNs in fgMakeBasicBlocks rather than recounting afterwards (perf)

With current branch -0.88 % diff improvement

Summary:
(Note: Lower is better)

Total bytes of diff: -27333 (-0.88 % of base)
    diff is an improvement.

Total byte diff includes -30363 bytes from reconciling methods
        Base had  434 unique methods,    30363 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
      -27333 : System.Private.CoreLib.dasm (-0.88 % of base)

1 total files with size differences.

Top method regessions by size (bytes):
         679 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.ICollection.CopyTo(ref,int):this
         661 : System.Private.CoreLib.dasm - KeyCollection:System.Collections.ICollection.CopyTo(ref,int):this
         660 : System.Private.CoreLib.dasm - ValueCollection:System.Collections.ICollection.CopyTo(ref,int):this
         642 : System.Private.CoreLib.dasm - Enumerator:System.Collections.IEnumerator.get_Current():ref:this
         624 : System.Private.CoreLib.dasm - ReadOnlyCollection`1:System.Collections.ICollection.CopyTo(ref,int):this

Top method improvements by size (bytes):
       -1311 : System.Private.CoreLib.dasm - __Error:WinIOError(int,ref)
        -955 : System.Private.CoreLib.dasm - EventProvider:WriteEvent(byref,long,long,ref):bool:this
        -712 : System.Private.CoreLib.dasm - ManifestBasedResourceGroveler:HandleSatelliteMissing():this
        -644 : System.Private.CoreLib.dasm - List`1:CopyTo(int,ref,int,int):this
        -580 : System.Private.CoreLib.dasm - EncoderExceptionFallbackBuffer:Fallback(char,char,int):bool:this

1000 total methods with size differences.

With #6634 from -1.44% to -2.49%

Total bytes of diff: -77277 (-2.49 % of base)
    diff is an improvement.

Total byte diff includes -29108 bytes from reconciling methods
        Base had  414 unique methods,    29108 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
      -77277 : System.Private.CoreLib.dasm (-2.49 % of base)

1 total files with size differences.

Top method regessions by size (bytes):
         642 : System.Private.CoreLib.dasm - Enumerator:System.Collections.IEnumerator.get_Current():ref:this
         624 : System.Private.CoreLib.dasm - ReadOnlyCollection`1:System.Collections.ICollection.CopyTo(ref,int):this
         418 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct):this
         389 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.ICollection.CopyTo(ref,int):this
         371 : System.Private.CoreLib.dasm - KeyCollection:System.Collections.ICollection.CopyTo(ref,int):this

Top method improvements by size (bytes):
       -3288 : System.Private.CoreLib.dasm - Array:Sort(ref,int,int,ref)
       -2214 : System.Private.CoreLib.dasm - Array:LastIndexOf(ref,struct,int,int):int
       -1540 : System.Private.CoreLib.dasm - Array:IndexOf(ref,struct,int,int):int
       -1311 : System.Private.CoreLib.dasm - __Error:WinIOError(int,ref)
       -1255 : System.Private.CoreLib.dasm - EventProvider:WriteEvent(byref,long,long,ref):bool:this

1339 total methods with size differences.

Resolves https://github.com/dotnet/coreclr/issues/6744

@mikedn @AndyAyersMS @kyulee1

mikedn · 2016-08-14T13:39:22Z

I'm not sure what you are trying to do. fgMakeBasicBlocks doesn't cause inlining to abort so moving return counting to it doesn't solve the problem (though it's probably a good thing to do for performance reasons).

It's fgFindJumpTargets which decides that inlining isn't profitable and aborts inlining early, before we get a chance to detect "no return" and mark the call node as such. It's also possible to abort inlining even earlier than fgFindJumpTargets for various reasons (most interesting being IL code size) but AFAIR in the case of ThrowArgumentOutOfRangeException the problem was fgFindJumpTargets.

So, we would probably need to move return counting to fgFindJumpTargets and mark the call node with GTF_CALL_M_DOES_NOT_RETURN as soon as possible, before inlining is aborted. Unfortunately this isn't as trivial as it may seem, we need to also look at calls and reproduce the tail call logic from fgMakeBasicBlocks to ensure that we're counting all return blocks.

And even if we do that there's still the possibility to find more returns than there are because fgMakeBasicBlocks ignores unreachable code while fgFindJumpTargets does not. Well, the C# compiler doesn't appear to produce such unreachable code so maybe this is not something we need to worry about.

benaadams · 2016-08-14T17:12:20Z

Adding the check to fgFindJumpTargets still gives the same fail reasons

Inlines into 06002A5B ArraySegment`1:.ctor(ref,int,int):this
  [0 IL=0004 TR=000067 06000C70] [FAILED: does not return] ThrowHelper:ThrowArgumentNullException(int)
  [0 IL=0016 TR=000061 06000C74] [FAILED: unprofitable inline] ThrowHelper:ThrowArgumentOutOfRangeException(int,int)
  [0 IL=0028 TR=000054 06000C74] [FAILED: unprofitable inline] ThrowHelper:ThrowArgumentOutOfRangeException(int,int)
  [0 IL=0043 TR=000048 06000C6E] [FAILED: unprofitable inline] ThrowHelper:ThrowArgumentException(int)
Budget: initialTime=270, finalTime=270, initialBudget=2700, currentBudget=2700
Budget: initialSize=1727, finalSize=1727

But does produce the moved calls:

; Assembly listing for method ArraySegment`1:.ctor(ref,int,int):this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 this         [V00,T00] (  5,   5  )   byref  ->  rsi         this
;* V01 TypeCtx      [V01    ] (  0,   0  )    long  ->  zero-ref   
;  V02 arg1         [V02,T01] (  5,   5  )     ref  ->   r8        
;  V03 arg2         [V03,T02] (  5,   5  )     int  ->  rdi        
;  V04 arg3         [V04,T03] (  3,   3  )     int  ->  rbx        
;  V05 loc0         [V05    ] (  1,   1  )  lclBlk (32) [rsp+0x00]  
;
; Lcl frame size = 32
G_M41507_IG01:
       push     rdi
       push     rsi
       push     rbx
       sub      rsp, 32
       mov      rsi, rcx
       mov      edi, r9d
       mov      ebx, dword ptr [rsp+60H]
G_M41507_IG02:
       test     r8, r8
       je       G_M41507_IG08
G_M41507_IG03:
       test     edi, edi
       jl       G_M41507_IG09
G_M41507_IG04:
       test     ebx, ebx
       jl       G_M41507_IG10
G_M41507_IG05:
       mov      edx, dword ptr [r8+8]
       sub      edx, edi
       cmp      edx, ebx
       jl       G_M41507_IG11
G_M41507_IG06:
       mov      rcx, rsi
       mov      rdx, r8
       call     CORINFO_HELP_CHECKED_ASSIGN_REF
       mov      dword ptr [rsi+8], edi
       mov      dword ptr [rsi+12], ebx
G_M41507_IG07:
       add      rsp, 32
       pop      rbx
       pop      rsi
       pop      rdi
       ret      
************** Beginning of cold code **************
G_M41507_IG08:
       mov      ecx, 3
       call     ThrowHelper:ThrowArgumentNullException(int)
G_M41507_IG09:
       mov      ecx, 41
       mov      edx, 4
       call     ThrowHelper:ThrowArgumentOutOfRangeException(int,int)
G_M41507_IG10:
       mov      ecx, 16
       mov      edx, 4
       call     ThrowHelper:ThrowArgumentOutOfRangeException(int,int)
G_M41507_IG11:
       mov      ecx, 23
       call     ThrowHelper:ThrowArgumentException(int)
       int3     
; Total bytes of code 132, prolog size 7 for method ArraySegment`1:.ctor(ref,int,int):this

Will clean it up, and add commit.

benaadams · 2016-08-14T17:23:59Z

@mikedn did you mean something like that?

benaadams · 2016-08-14T17:40:16Z

With #6634

Total bytes of diff: -44551 (-1.44 % of base)
    diff is an improvement.

Total byte diff includes 562 bytes from reconciling methods
        Base had    1 unique methods,       76 unique bytes
        Diff had    9 unique methods,      638 unique bytes

Top file improvements by size (bytes):
      -44551 : System.Private.CoreLib.dasm (-1.44 % of base)

1 total files with size differences.

Top method regessions by size (bytes):
         418 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct):this
         289 : System.Private.CoreLib.dasm - CustomAttributeData:Init(ref):this
         234 : System.Private.CoreLib.dasm - TimeSpan:TryParseExact(ref,ref,ref,byref):bool
         229 : System.Private.CoreLib.dasm - DateTimeParse:ParseByFormat(byref,byref,byref,ref,byref):bool
         187 : System.Private.CoreLib.dasm - DateTimeFormat:FormatCustomized(struct,ref,ref,struct):ref

Top method improvements by size (bytes):
       -3336 : System.Private.CoreLib.dasm - Array:Sort(ref,int,int,ref)
       -2396 : System.Private.CoreLib.dasm - Array:LastIndexOf(ref,struct,int,int):int
       -1677 : System.Private.CoreLib.dasm - Array:IndexOf(ref,struct,int,int):int
       -1251 : System.Private.CoreLib.dasm - Array:BinarySearch(ref,int,int,struct,ref):int
        -884 : System.Private.CoreLib.dasm - Comparer`1:Create(ref):ref

389 total methods with size differences.

Changes to

Summary:
(Note: Lower is better)

Total bytes of diff: -68026 (-2.19 % of base)
    diff is an improvement.

Total byte diff includes -29083 bytes from reconciling methods
        Base had  413 unique methods,    29083 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
      -68026 : System.Private.CoreLib.dasm (-2.19 % of base)

1 total files with size differences.

Top method regessions by size (bytes):
         679 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.ICollection.CopyTo(ref,int):this
         661 : System.Private.CoreLib.dasm - KeyCollection:System.Collections.ICollection.CopyTo(ref,int):this
         660 : System.Private.CoreLib.dasm - ValueCollection:System.Collections.ICollection.CopyTo(ref,int):this
         642 : System.Private.CoreLib.dasm - Enumerator:System.Collections.IEnumerator.get_Current():ref:this
         624 : System.Private.CoreLib.dasm - ReadOnlyCollection`1:System.Collections.ICollection.CopyTo(ref,int):this

Top method improvements by size (bytes):
       -3048 : System.Private.CoreLib.dasm - Array:Sort(ref,int,int,ref)
       -2214 : System.Private.CoreLib.dasm - Array:LastIndexOf(ref,struct,int,int):int
       -1540 : System.Private.CoreLib.dasm - Array:IndexOf(ref,struct,int,int):int
       -1311 : System.Private.CoreLib.dasm - __Error:WinIOError(int,ref)
       -1143 : System.Private.CoreLib.dasm - Array:BinarySearch(ref,int,int,struct,ref):int

1328 total methods with size differences.

benaadams · 2016-08-14T17:46:21Z

With current branch

Summary:
(Note: Lower is better)

Total bytes of diff: -27333 (-0.88 % of base)
    diff is an improvement.

Total byte diff includes -30363 bytes from reconciling methods
        Base had  434 unique methods,    30363 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
      -27333 : System.Private.CoreLib.dasm (-0.88 % of base)

1 total files with size differences.

Top method regessions by size (bytes):
         679 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.ICollection.CopyTo(ref,int):this
         661 : System.Private.CoreLib.dasm - KeyCollection:System.Collections.ICollection.CopyTo(ref,int):this
         660 : System.Private.CoreLib.dasm - ValueCollection:System.Collections.ICollection.CopyTo(ref,int):this
         642 : System.Private.CoreLib.dasm - Enumerator:System.Collections.IEnumerator.get_Current():ref:this
         624 : System.Private.CoreLib.dasm - ReadOnlyCollection`1:System.Collections.ICollection.CopyTo(ref,int):this

Top method improvements by size (bytes):
       -1311 : System.Private.CoreLib.dasm - __Error:WinIOError(int,ref)
        -955 : System.Private.CoreLib.dasm - EventProvider:WriteEvent(byref,long,long,ref):bool:this
        -712 : System.Private.CoreLib.dasm - ManifestBasedResourceGroveler:HandleSatelliteMissing():this
        -644 : System.Private.CoreLib.dasm - List`1:CopyTo(int,ref,int,int):this
        -580 : System.Private.CoreLib.dasm - EncoderExceptionFallbackBuffer:Fallback(char,char,int):bool:this

1000 total methods with size differences.

benaadams · 2016-08-14T17:49:08Z

With List1:CopyTo(int,ref,int,int):this` changing from

; Assembly listing for method List`1:CopyTo(int,ref,int,int):this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 this         [V00,T00] (  4,   4  )     ref  ->  rsi         this
;  V01 arg1         [V01,T01] (  4,   4  )     int  ->  rdi        
;  V02 arg2         [V02,T02] (  3,   3  )     ref  ->  rbx        
;  V03 arg3         [V03,T03] (  3,   3  )     int  ->  rbp        
;  V04 arg4         [V04,T05] (  2,   2  )     int  ->  r14        
;  V05 tmp0         [V05,T04] (  2,   4  )     ref  ->  rcx        
;  V06 OutArgs      [V06    ] (  1,   1  )  lclBlk (48) [rsp+0x00]  
;
; Lcl frame size = 48
G_M45633_IG01:
       push     r14
       push     rdi
       push     rsi
       push     rbp
       push     rbx
       sub      rsp, 48
       mov      rsi, rcx
       mov      edi, edx
       mov      rbx, r8
       mov      ebp, r9d
       mov      r14d, dword ptr [rsp+80H]
G_M45633_IG02:
       mov      ecx, dword ptr [rsi+24]
       sub      ecx, edi
       cmp      ecx, r14d
       jge      SHORT G_M45633_IG03
       mov      ecx, 23
       call     ThrowHelper:ThrowArgumentException(int)
G_M45633_IG03:
       mov      rcx, gword ptr [rsi+8]
       mov      dword ptr [rsp+20H], r14d
       xor      edx, edx
       mov      dword ptr [rsp+28H], edx
       mov      edx, edi
       mov      r8, rbx
       mov      r9d, ebp
       call     Array:Copy(ref,int,ref,int,int,bool)
       nop      
G_M45633_IG04:
       add      rsp, 48
       pop      rbx
       pop      rbp
       pop      rsi
       pop      rdi
       pop      r14
       ret      
; Total bytes of code 89, prolog size 10 for method List`1:CopyTo(int,ref,int,int):this

To

; Assembly listing for method List`1:CopyTo(int,ref,int,int):this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 this         [V00,T00] (  4,   4  )     ref  ->  rcx         this
;  V01 arg1         [V01,T01] (  4,   4  )     int  ->  rdx        
;  V02 arg2         [V02,T02] (  3,   3  )     ref  ->   r8        
;  V03 arg3         [V03,T03] (  3,   3  )     int  ->   r9        
;  V04 arg4         [V04,T05] (  2,   2  )     int  ->  rax        
;  V05 tmp0         [V05,T04] (  2,   4  )     ref  ->  rcx        
;  V06 OutArgs      [V06    ] (  1,   1  )  lclBlk (48) [rsp+0x00]  
;
; Lcl frame size = 56
G_M45633_IG01:
       sub      rsp, 56
       nop      
       mov      eax, dword ptr [rsp+60H]
G_M45633_IG02:
       mov      r10d, dword ptr [rcx+24]
       sub      r10d, edx
       cmp      r10d, eax
       jl       G_M45633_IG05
G_M45633_IG03:
       mov      rcx, gword ptr [rcx+8]
       mov      dword ptr [rsp+20H], eax
       xor      eax, eax
       mov      dword ptr [rsp+28H], eax
       call     Array:Copy(ref,int,ref,int,int,bool)
       nop      
G_M45633_IG04:
       add      rsp, 56
       ret      
************** Beginning of cold code **************
G_M45633_IG05:
       mov      ecx, 23
       call     ThrowHelper:ThrowArgumentException(int)
       int3     
; Total bytes of code 61, prolog size 5 for method List`1:CopyTo(int,ref,int,int):this

Inlines into 06003630 List`1:CopyTo(int,ref,int,int):this
  [0 IL=0014 TR=000027 06000C2E] [FAILED: unprofitable inline] ThrowHelper:ThrowArgumentException(int)
  [1 IL=0030 TR=000016 06002B05] [below ALWAYS_INLINE size] Array:Copy(ref,int,ref,int,int)
    [0 IL=0007 TR=000033 06002B06] [FAILED: noinline per IL/cached result] Array:Copy(ref,int,ref,int,int,bool)
Budget: initialTime=168, finalTime=180, initialBudget=1680, currentBudget=1680
Budget: initialSize=952, finalSize=952
Successfully inlined Array:Copy(ref,int,ref,int,int) (13 IL bytes) (depth 1) [below ALWAYS_INLINE size]

mikedn · 2016-08-14T18:25:56Z

Adding the check to fgFindJumpTargets still gives the same fail reasons

Yes, that's normal. Currently the reason why inlining fails is irrelevant, it can be "no return" or any other. What matters is marking the call node as "no return".

mikedn · 2016-08-14T18:27:40Z

src/jit/flowgraph.cpp

@@ -4857,7 +4859,8 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, BYTE*
                    fgObserveInlineConstants(opcode, pushedStack, isInlining);
                }
                break;
-
+            case CEE_RET:
+                retBlocks++;


As already mentioned, this is not enough. We need to handle tail calls as well, they count as returns.

Is always including CEE_JMP enough? (made change)

Nope, CEE_JMP is needed but we also need to look for calls with a tailcall prefix.

Well out of my depth now... rename to retBlocks->potentialRetBlocks and include CEE_TAILCALL?

Or does the called function need to be checked?

Note sure, need to look more at what fgMakeBasicBlocks is doing. Maybe we can actually ignore the .tail prefix, the IL code should contain a ret anyway for the method to be valid.

Yeah, there has to be a ret after a tail.call. If there's no ret then the JIT fails to compile the method so we don't need to be concerned about what happens if someone feeds the JIT IL code that fails to conform to the ECMA spec. An invalid method would be flagged as "no return" and that's OK, calling such a method would result in an InvalidProgramException being thrown.

So counting CEE_RET and CEE_JMP is enough, if there aren't any then the method doesn't return (either because it throws or because it contains an infinite loop).

mikedn · 2016-08-14T18:33:48Z

Heh, that List.CopyTo is a fine example of the mess missing "no return" information can produce, no less than 5 registers saved/restored!

benaadams · 2016-08-14T18:50:57Z

Reverted to only make the observations when doing inlining observations; and seems to produce the same outcomes.

benaadams · 2016-08-14T19:35:05Z

The outcomes are the same with the later changes (so List1:CopyTo is still much improved); it doesn't seem to precompile __Error:WinIOError(int,ref) anymore, so looking at the next highest drop off EventProvider:WriteEvent(byref,long,long,ref):bool:this

... removed pre-asm and post-asm (improved) as was vast ...

Lot of repeats of the same block in the cold code in new (no cold code in old)

mikedn · 2016-08-14T19:42:49Z

so looking at the next highest drop off EventProvider:WriteEvent

Yeah, I've seen that monster before 😄

benaadams · 2016-08-14T19:51:18Z

Squashed...

@mikedn I assume deduping the cold chunks wouldn't be straightforward? Would be nice to convert the 30 identical ones to 1...

benaadams · 2016-08-14T19:59:31Z

Compiler::fgUpdateFlowGraph(bool doTailDuplication) seems to do something in this area...

benaadams · 2016-08-14T20:17:17Z

Think will leave alone...

mikedn · 2016-08-14T20:47:14Z

I assume deduping the cold chunks wouldn't be straightforward? Would be nice to convert the 30 to 1...

I don't think there's anything in the JIT that could do that at the moment. Normally it would be relatively expensive to do as the compiler would need to compare the blocks to figure out that they are identical. But given in this particular case it would be easy to tell that they are identical - they come from the same method and same IL offset. Anyway, this is an entirely different story.

Compiler::fgUpdateFlowGraph(bool doTailDuplication) seems to do something in this area...

Tail duplication is sort of the opposite - if there are multiple jumps to the same block then the jumps are replaced with the block's code. This eliminates the jumps at the cost of increased code size so it's done only when the block's code is small.

mikedn · 2016-08-15T09:22:11Z

src/jit/flowgraph.cpp

+                // Mark the call node as "no return" as it can impact caller's code quality.
+                impInlineInfo->iciCall->gtCallMoreFlags |= GTF_CALL_M_DOES_NOT_RETURN;
+
+                if (compInlineResult->IsFailure())


The IsFailure check should come after NoteBool, otherwise it is useless.

Considering that CALLEE_DOES_NOT_RETURN doesn't currently cause inlining to be aborted right away it might be best to leave this stuff out, the existing "no return" code will do this anyway. All we really need here is to mark the call node as "no return".

Also, it might be good to add an assert in the existing "no return" code to ensure that if fgFindJumpTargets marked the call as "no return" then there really are no BBJ_RETURN blocks in the method.

I can add an assert in EnhancedLegacyPolicy::NoteBool, but ideally would check it immediately after fgMakeBasicBlocks, but not sure I can inspect the observation there?

can add an assert in EnhancedLegacyPolicy::NoteBool

Note there, the assert should be in fgFindBasicBlocks where the rest of the no return code is.

but not sure I can inspect the observation there

This has nothing to do with the inline observation, it has to do with the call flag.

Something like?

unsigned retBlocks = fgMakeBasicBlocks(info.compCode, info.compILCodeSize, jumpTarget); noway_assert(retBlocks > 0 && ((impInlineInfo->iciCall->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0));

mikedn · 2016-08-15T18:13:11Z

src/jit/flowgraph.cpp

+                // Mark the call node as "no return" as it can impact caller's code quality.
+                impInlineInfo->iciCall->gtCallMoreFlags |= GTF_CALL_M_DOES_NOT_RETURN;
+            }
+            compInlineResult->NoteBool(InlineObservation::CALLEE_DOES_NOT_RETURN, true);


This is not needed.

pgavlin · 2016-08-15T20:19:15Z

@dotnet/jit-contrib PTAL

benaadams · 2016-08-19T11:08:06Z

Renamed both issue and PR to be more functionally descriptive. Seem ok?

mikedn · 2016-08-19T11:16:00Z

Isn't fgFindJumpTargets prejit/Pre-import stage? Am not 100% on the terminology, there is a comment about prejit in the caller and the function

"prejit" is ngen/crossgen.

Renamed both issue and PR to be more functionally descriptive. Seem ok?

👍

benaadams · 2016-08-19T11:46:18Z

Not sure why it segfaults on Ubuntu/osx checked and CentOS7.1 debug, guessing that would suggests compInlineResult or impInlineInfo->iciCall is null?

Or something completely unrealted...

mikedn · 2016-08-19T12:37:16Z

Not sure why it segfaults on Ubuntu/osx checked and CentOS7.1 debug, guessing that would suggests compInlineResult or impInlineInfo->iciCall is null?

Yep, impInlineInfo may be null during ngen. You need to check isInlining before setting that flag.

It's strange that this doesn't crash the Windows builds as well.

benaadams · 2016-08-20T00:30:55Z

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test
@dotnet-bot test Windows_NT x86 legacy_backend Checked Build and Test
@dotnet-bot test Windows_NT x86 ryujit Checked Build and Test

benaadams · 2016-08-20T01:50:21Z

Merge clash with #6793

benaadams · 2016-08-20T03:47:44Z

@mikedn got there in the end, thank you for all your help 😄

benaadams · 2016-08-20T03:57:54Z

@erozenfeld we had clash in fgMakeBasicBlocks and fgFindBasicBlocks; this change seem compatible?

benaadams · 2016-08-20T04:04:46Z

src/jit/flowgraph.cpp


    if (compIsForInlining())
    {
-        if (compInlineResult->IsFailure())


Uses fail after setting no return flag

mikedn · 2016-08-20T05:08:54Z

It's probably best to leave that conditional return where it was for the sake of clarity, it communicates the fact that fgMakeBasicBlocks can make fatal observation.

We could remove the conditional return that's after the CALEE_DOES_NOT_RETURN observation because this observation doesn't currently cause inlining to fail right away. But maybe it will in the future and then the return will be useful so let's leave that one where it is as well.

benaadams · 2016-08-20T05:31:13Z

@erozenfeld @mikedn added the fail back

benaadams · 2016-08-20T10:02:04Z

@dotnet-bot test Windows_NT x64 Debug Build and Test

benaadams · 2016-09-07T08:12:45Z

@AndyAyersMS any thoughts about this? Most of the major items it effects have been merged:

ArraySegment exceptions to ThrowHelper ArraySegment<T> exceptions to ThrowHelper #7058
Array exceptions to ThrowHelper Array<T> exceptions to ThrowHelper #7059
Task Exceptions to ThrowHelper Task<T> Exceptions to ThrowHelper #7076
Improve ThrowHelper Improve ThrowHelper #7048

pgavlin · 2016-09-07T15:48:58Z

@AndyAyersMS is out at the moment--@kyulee1, can you take a look?

benaadams · 2016-10-11T23:44:46Z

Rebased

benaadams · 2016-10-12T02:02:25Z

Hmmm.. this doesn't seem to work anymore and leaves List1:CopyTo(int,ref,int,int):this` the same before and after

benaadams · 2016-10-12T08:39:28Z

Replacing with #7580

dnfclas added the cla-already-signed label Aug 14, 2016

mikedn reviewed Aug 14, 2016
View reviewed changes

benaadams changed the title ~~Always make Jit No Return Observation~~ Observe No Return in prejit Aug 14, 2016

benaadams force-pushed the jit-obs-ret branch 2 times, most recently from d44d011 to c65d749 Compare August 14, 2016 19:47

benaadams mentioned this pull request Aug 14, 2016

Remove Generic<T> Exception code repetition #6634

Closed

mikedn reviewed Aug 15, 2016
View reviewed changes

benaadams force-pushed the jit-obs-ret branch from f4ba55c to 58e8b7d Compare August 15, 2016 18:10

mikedn reviewed Aug 15, 2016
View reviewed changes

benaadams force-pushed the jit-obs-ret branch 2 times, most recently from 4ac938c to 9fa2dca Compare August 15, 2016 19:16

benaadams force-pushed the jit-obs-ret branch from 19a8676 to cbd54bb Compare August 17, 2016 04:48

benaadams changed the title ~~Set DOES_NOT_RETURN gtCallMoreFlags in prejit~~ Set DOES_NOT_RETURN gtCallMoreFlags in fgFindJumpTargets Aug 19, 2016

benaadams force-pushed the jit-obs-ret branch from cbd54bb to d481ca5 Compare August 19, 2016 21:33

benaadams force-pushed the jit-obs-ret branch from d481ca5 to f158e25 Compare August 20, 2016 01:54

benaadams reviewed Aug 20, 2016
View reviewed changes

src/jit/flowgraph.cpp

if (compIsForInlining())

{

if (compInlineResult->IsFailure())

Copy link

Member Author

benaadams Aug 20, 2016

Choose a reason for hiding this comment

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

Uses fail after setting no return flag

benaadams force-pushed the jit-obs-ret branch from 92d2491 to d1d4093 Compare August 20, 2016 09:28

pgavlin added the area-CodeGen label Aug 22, 2016

benaadams mentioned this pull request Aug 31, 2016

Reduce code bloat around formatted GetResourceString calls #7007

Closed

benaadams added 2 commits October 12, 2016 00:42

Count BBJ_RETURN in fgMakeBasicBlocks

914774c

Set DOES_NOT_RETURN flag in fgFindJumpTargets

7cb8db3

benaadams force-pushed the jit-obs-ret branch from d1d4093 to 7cb8db3 Compare October 11, 2016 23:43

benaadams closed this Oct 12, 2016

benaadams mentioned this pull request Oct 12, 2016

Flag does_not_return early #7580

Merged

benaadams deleted the jit-obs-ret branch March 27, 2018 05:11

Set DOES_NOT_RETURN gtCallMoreFlags in fgFindJumpTargets #6742

Set DOES_NOT_RETURN gtCallMoreFlags in fgFindJumpTargets #6742

Conversation

benaadams commented Aug 14, 2016 • edited Loading

mikedn commented Aug 14, 2016

benaadams commented Aug 14, 2016

benaadams commented Aug 14, 2016

benaadams commented Aug 14, 2016

benaadams commented Aug 14, 2016

benaadams commented Aug 14, 2016 • edited Loading

mikedn commented Aug 14, 2016

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

mikedn Aug 14, 2016 • edited Loading

Choose a reason for hiding this comment

mikedn commented Aug 14, 2016

benaadams commented Aug 14, 2016

benaadams commented Aug 14, 2016 • edited Loading

mikedn commented Aug 14, 2016

benaadams commented Aug 14, 2016 • edited Loading

benaadams commented Aug 14, 2016

benaadams commented Aug 14, 2016

mikedn commented Aug 14, 2016

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

pgavlin commented Aug 15, 2016

benaadams commented Aug 19, 2016

mikedn commented Aug 19, 2016

benaadams commented Aug 19, 2016 • edited Loading

mikedn commented Aug 19, 2016

benaadams commented Aug 20, 2016

benaadams commented Aug 20, 2016

benaadams commented Aug 20, 2016 • edited Loading

benaadams commented Aug 20, 2016

Choose a reason for hiding this comment

mikedn commented Aug 20, 2016

benaadams commented Aug 20, 2016

benaadams commented Aug 20, 2016

benaadams commented Sep 7, 2016

pgavlin commented Sep 7, 2016

benaadams commented Oct 11, 2016

benaadams commented Oct 12, 2016

benaadams commented Oct 12, 2016 • edited Loading

benaadams commented Aug 14, 2016 •

edited

Loading

benaadams commented Aug 14, 2016 •

edited

Loading

mikedn Aug 14, 2016 •

edited

Loading

benaadams commented Aug 14, 2016 •

edited

Loading

benaadams commented Aug 14, 2016 •

edited

Loading

benaadams commented Aug 19, 2016 •

edited

Loading

benaadams commented Aug 20, 2016 •

edited

Loading

benaadams commented Oct 12, 2016 •

edited

Loading