JIT: Allow helper calls that always throw to be marked as no-return #100900

amanasifkhalid · 2024-04-11T02:57:55Z

Fixes #100458 by addressing two issues:

When flagging a call node as no-return with GTF_CALL_M_DOES_NOT_RETURN, we should always increment Compiler::optNoReturnCallCount to avoid asserts in Compiler::fgTailMergeThrows. Previously, we weren't doing this in a unified place, which seemed error-prone.
When incrementing the optNoReturnCallCount member of an inlinee compiler, ensure this information is propagated to the inlinee's parent compiler. In a similar vein, if we try to inline a call, and the inlinee compiler determines it does not return, make sure we increment optNoReturnCallCount in the parent compiler object if the inline fails -- we've kept the call, and we now know it doesn't return.

With these changes, I can now mark helper calls that always throw as no-return; this unblocks morph to convert BBJ_ALWAYS blocks with helper calls that throw into BBJ_THROW blocks, and has the nice side effect of improving the efficacy of throw merging. Since I was touching relevant code, I decided to improve our usage of GenTreeCall::IsHelperCall, too.

Diffs -- quite extensive in libraries tests; not too surprising as they do a lot of exceptional case validation.

dotnet-policy-service · 2024-04-11T02:58:36Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

amanasifkhalid · 2024-04-11T10:43:55Z

cc @dotnet/jit-contrib, @AndyAyersMS PTAL. Nice size improvements.

src/coreclr/jit/compiler.h

AndyAyersMS · 2024-04-11T21:56:55Z

I think I'd prefer to see the inlining aspects get handled in the same way we handle other cases of inlinee->root propagation:

runtime/src/coreclr/jit/fginline.cpp

Lines 1522 to 1610 in eb4bf70

    
               // 
        
               // At this point, we have successully inserted inlinee's code. 
        
               // 
        
               // 
        
               // Copy out some flags 
        
               // 
        
               compLongUsed |= InlineeCompiler->compLongUsed; 
        
               compFloatingPointUsed |= InlineeCompiler->compFloatingPointUsed; 
        
               compLocallocUsed |= InlineeCompiler->compLocallocUsed; 
        
               compLocallocOptimized |= InlineeCompiler->compLocallocOptimized; 
        
               compQmarkUsed |= InlineeCompiler->compQmarkUsed; 
        
               compGSReorderStackLayout |= InlineeCompiler->compGSReorderStackLayout; 
        
               compHasBackwardJump |= InlineeCompiler->compHasBackwardJump; 
        
               lvaGenericsContextInUse |= InlineeCompiler->lvaGenericsContextInUse; 
        
           #ifdef TARGET_ARM64 
        
               info.compNeedsConsecutiveRegisters |= InlineeCompiler->info.compNeedsConsecutiveRegisters; 
        
           #endif 
        
               // If the inlinee compiler encounters switch tables, disable hot/cold splitting in the root compiler. 
        
               // TODO-CQ: Implement hot/cold splitting of methods with switch tables. 
        
               if (InlineeCompiler->fgHasSwitch && opts.compProcedureSplitting) 
        
               { 
        
                   opts.compProcedureSplitting = false; 
        
                   JITDUMP("Turning off procedure splitting for this method, as inlinee compiler encountered switch tables; " 
        
                           "implementation limitation.\n"); 
        
               } 
        
           #ifdef FEATURE_SIMD 
        
               if (InlineeCompiler->usesSIMDTypes()) 
        
               { 
        
                   setUsesSIMDTypes(true); 
        
               } 
        
           #endif // FEATURE_SIMD 
        
               // Update unmanaged call details 
        
               info.compUnmanagedCallCountWithGCTransition += InlineeCompiler->info.compUnmanagedCallCountWithGCTransition; 
        
               // Update stats for inlinee PGO 
        
               // 
        
               if (InlineeCompiler->fgPgoSchema != nullptr) 
        
               { 
        
                   fgPgoInlineePgo++; 
        
               } 
        
               else if (InlineeCompiler->fgPgoFailReason != nullptr) 
        
               { 
        
                   // Single block inlinees may not have probes 
        
                   // when we've ensabled minimal profiling (which 
        
                   // is now the default). 
        
                   // 
        
                   if (InlineeCompiler->fgBBcount == 1) 
        
                   { 
        
                       fgPgoInlineeNoPgoSingleBlock++; 
        
                   } 
        
                   else 
        
                   { 
        
                       fgPgoInlineeNoPgo++; 
        
                   } 
        
               } 
        
               // Update optMethodFlags 
        
           #ifdef DEBUG 
        
               unsigned optMethodFlagsBefore = optMethodFlags; 
        
           #endif 
        
               optMethodFlags |= InlineeCompiler->optMethodFlags; 
        
           #ifdef DEBUG 
        
               if (optMethodFlags != optMethodFlagsBefore) 
        
               { 
        
                   JITDUMP("INLINER: Updating optMethodFlags --  root:%0x callee:%0x new:%0x\n", optMethodFlagsBefore, 
        
                           InlineeCompiler->optMethodFlags, optMethodFlags); 
        
               } 
        
           #endif 
        
               // If an inlinee needs GS cookie we need to make sure that the cookie will not be allocated at zero stack offset. 
        
               // Note that if the root method needs GS cookie then this has already been taken care of. 
        
               if (!getNeedsGSSecurityCookie() && InlineeCompiler->getNeedsGSSecurityCookie()) 
        
               { 
        
                   setNeedsGSSecurityCookie(); 
        
                   const unsigned dummy         = lvaGrabTempWithImplicitUse(false DEBUGARG("GSCookie dummy for inlinee")); 
        
                   LclVarDsc*     gsCookieDummy = lvaGetDesc(dummy); 
        
                   gsCookieDummy->lvType        = TYP_INT; 
        
                   gsCookieDummy->lvIsTemp      = true; // It is not alive at all, set the flag to prevent zero-init. 
        
                   lvaSetVarDoNotEnregister(dummy DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr)); 
        
               }

So inlinee would transfer its no return call info to root only if the inline happens.

VSadov · 2024-04-12T01:10:13Z

src/coreclr/jit/codegenlinear.cpp

-                        if ((call->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0 ||
-                            ((call->AsCall()->gtCallType == CT_HELPER) &&
-                             Compiler::s_helperCallProperties.AlwaysThrow(call->AsCall()->GetHelperNum())))
+                        if (call->AsCall()->IsNoReturn())


Not related to this change, but I was wondering - why do we do (call->gtOper == GT_CALL)?
Is it possible at this point for a BBJ_THROW to end with anything other than a call?

I am mostly concerned about a possibility that the throwing call is somewhere inside the block and followed by some unreachable junk. Then we either emit the junk unnecessarily, or, worse, emitter may skip/optimize it and we end up with the same state we are trying to prevent here.
So I wonder - is there GT_SOMETHING that is not a call, and yet it throws and can conclude a BBJ_THROW?

I tested this out by asserting the last node is a call if the block has any IR, and the assert did hit. Here's some example IR it hit for:

------------ BB04 [0003] [024..02A) (throw), preds={BB03} succs={} N069 (???,???) [000043] ----------- IL_OFFSET void INLRT @ 0x024[E-] REG NA N071 ( 3, 10) [000046] Hc--------- t46 = CNS_INT(h) long 0x7ffc04235008 ftn REG NA /--* t46 long N073 ( 5, 12) [000047] nc--G------ t47 = * IND long REG NA /--* t47 long control expr N075 ( 14, 5) [000020] --CXG------ t20 = * CALL int Microsoft.FSharp.Core.Operators+OperatorIntrinsics:alreadyFinished[int]():int REG rax $c4 /--* t20 int N077 ( 15, 6) [000022] --CXG------ * RETURN int REG NA $VN.Void

That call is marked as not returning, but there's still a GT_RETURN after it; this seems to confirm your concerns. Perhaps we need a dead code elimination check somewhere that deletes any IR after no-return calls, though a quick-and-dirty fix might be to always emit the breakpoint after no-return calls (regardless of whether they're the last node or not) in CodeGen::genCall, or somewhere similar.

This method in particular expects an int to be return, so removing the GT_RETURN might trip up the JIT if we do it too early...

Morph has an exemption for tail calls that are no return, and will not prune the IR afterwards. So likely the method above is a tail call? Also there is this note:

runtime/src/coreclr/jit/morph.cpp

Lines 7825 to 7844 in 543b2cc

if (call->IsNoReturn())

{

//

// If we know that the call does not return then we can set fgRemoveRestOfBlock

// to remove all subsequent statements and change the call's basic block to BBJ_THROW.

// As a result the compiler won't need to preserve live registers across the call.

//

// This isn't need for tail calls as there shouldn't be any code after the call anyway.

// Besides, the tail call code is part of the epilog and converting the block to

// BBJ_THROW would result in the tail call being dropped as the epilog is generated

// only for BBJ_RETURN blocks.

//

if (!call->IsTailCall())

{

fgRemoveRestOfBlock = true;

}

}

return call;

which the IR dump above seems to contradict, since it looks from the dump that the block kind is BBJ_THROW. I wonder if the comment is now stale?

Earlier on morph will reject implicit tail calls to no return methods, I wonder if we should just do the same for explicit tail calls, even though technically we'd be violating the intent of the .tail prefix. Then we would not have these special cases.

a quick-and-dirty fix might be to always emit the breakpoint after no-return calls

There's also a concern that with profiler-driven rejitting a method that might always throw when the JIT looked at it could be updated to not always throw. I don't know if there is any good reason to support this sort of rejitting, but we should make sure there's an obvious failure if someone does it since the jitted code may depend on the old behavior. So I think in general we will want to put a breakpoint afterwards.

Morph has an exemption for tail calls that are no return, and will not prune the IR afterwards. So likely the method above is a tail call?

In the above case, the call is an implicit tail call to a no-return method, so fgMorphPotentialTailCall rejected it, and fgMorphCall set fgRemoveRestOfBlock = true. The GT_CALL node is an operand of a GT_RETURN node, which is in the last statement of the block, so fgRemoveRestOfBlock didn't actually remove any IR, but it still converted the block into a BBJ_THROW. Perhaps we could add a check that prevents converting the block into a BBJ_THROW if it ends with a GT_RETURN node, so that we can always expect the last node in a BBJ_THROW to be a GT_CALL? I'm not sure if we lose any benefits by doing this -- maybe we could still mark the block as rarely run?

If we decide to make any changes to this, I'll open a follow-up PR.

AndyAyersMS · 2024-04-12T17:24:56Z

Can you update the PR/initial comment above to reflect the latest?

amanasifkhalid · 2024-04-12T17:57:21Z

Can you update the PR/initial comment above to reflect the latest?

Sure thing.

amanasifkhalid · 2024-04-12T18:03:46Z

CI failures are unrelated System.Security.Cryptography test failures.

amanasifkhalid · 2024-04-12T18:15:08Z

/ba-g unrelated failures with tracking issues

…otnet#100900) Fixes dotnet#100458 by addressing two issues: When flagging a call node as no-return with GTF_CALL_M_DOES_NOT_RETURN, we should always increment Compiler::optNoReturnCallCount to avoid asserts in Compiler::fgTailMergeThrows. Previously, we weren't doing this in a unified place, which seemed error-prone. When incrementing the optNoReturnCallCount member of an inlinee compiler, ensure this information is propagated to the inlinee's parent compiler. In a similar vein, if we try to inline a call, and the inlinee compiler determines it does not return, make sure we increment optNoReturnCallCount in the parent compiler object if the inline fails -- we've kept the call, and we now know it doesn't return. With these changes, I can now mark helper calls that always throw as no-return; this unblocks morph to convert BBJ_ALWAYS blocks with helper calls that throw into BBJ_THROW blocks, and has the nice side effect of improving the efficacy of throw merging. Since I was touching relevant code, I decided to improve our usage of GenTreeCall::IsHelperCall, too.

amanasifkhalid added 3 commits April 10, 2024 22:14

Handle helper methods that don't return

a98d9aa

IsHelperCall cleanup

9559251

Style

03b47ac

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 11, 2024

dotnet-policy-service bot assigned amanasifkhalid Apr 11, 2024

EgorBo reviewed Apr 11, 2024

View reviewed changes

src/coreclr/jit/compiler.h Outdated Show resolved Hide resolved

EgorBo approved these changes Apr 11, 2024

View reviewed changes

amanasifkhalid requested a review from AndyAyersMS April 11, 2024 21:01

Propagate optNoReturnCallCount from inlinee compiler

3954364

VSadov reviewed Apr 12, 2024

View reviewed changes

build-analysis bot mentioned this pull request Apr 12, 2024

System.Net.Tests.HttpWebRequestTest_Async.GetResponseAsync_ParametersAreNotCachable_CreateNewClient test fails #100912

Closed

VSadov mentioned this pull request Apr 12, 2024

Allow async interruptions on safepoints #95565

Merged

AndyAyersMS approved these changes Apr 12, 2024

View reviewed changes

amanasifkhalid mentioned this pull request Apr 12, 2024

System.Security.Cryptography.Tests.FactorySha1Tests.HashAlgorithm_TransformBlockAndDispose_ConcurrentUseDoesNotCrashProcess fails #100983

Closed

amanasifkhalid merged commit 2c824ae into dotnet:main Apr 12, 2024
107 of 110 checks passed

amanasifkhalid deleted the bbj-always-throw branch April 12, 2024 19:06

VSadov mentioned this pull request Apr 15, 2024

Insert a breakpoint instruction after throwing calls #101037

Closed

AndyAyersMS mentioned this pull request Apr 16, 2024

[Perf] Windows/x64: 1 Improvement on 4/12/2024 3:02:01 AM dotnet/perf-autofiling-issues#32749

Open

github-actions bot locked and limited conversation to collaborators May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Allow helper calls that always throw to be marked as no-return #100900

JIT: Allow helper calls that always throw to be marked as no-return #100900

amanasifkhalid commented Apr 11, 2024 •

edited

Loading

dotnet-policy-service bot commented Apr 11, 2024

amanasifkhalid commented Apr 11, 2024

AndyAyersMS commented Apr 11, 2024

VSadov Apr 12, 2024 •

edited

Loading

VSadov Apr 12, 2024

amanasifkhalid Apr 12, 2024 •

edited

Loading

AndyAyersMS Apr 12, 2024

amanasifkhalid Apr 12, 2024 •

edited

Loading

AndyAyersMS commented Apr 12, 2024

amanasifkhalid commented Apr 12, 2024

amanasifkhalid commented Apr 12, 2024

amanasifkhalid commented Apr 12, 2024

	if (call->IsNoReturn())
	{
	//
	// If we know that the call does not return then we can set fgRemoveRestOfBlock
	// to remove all subsequent statements and change the call's basic block to BBJ_THROW.
	// As a result the compiler won't need to preserve live registers across the call.
	//
	// This isn't need for tail calls as there shouldn't be any code after the call anyway.
	// Besides, the tail call code is part of the epilog and converting the block to
	// BBJ_THROW would result in the tail call being dropped as the epilog is generated
	// only for BBJ_RETURN blocks.
	//

	if (!call->IsTailCall())
	{
	fgRemoveRestOfBlock = true;
	}
	}

	return call;

JIT: Allow helper calls that always throw to be marked as no-return #100900

JIT: Allow helper calls that always throw to be marked as no-return #100900

Conversation

amanasifkhalid commented Apr 11, 2024 • edited Loading

dotnet-policy-service bot commented Apr 11, 2024

amanasifkhalid commented Apr 11, 2024

AndyAyersMS commented Apr 11, 2024

VSadov Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

VSadov Apr 12, 2024

Choose a reason for hiding this comment

amanasifkhalid Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

AndyAyersMS Apr 12, 2024

Choose a reason for hiding this comment

amanasifkhalid Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

AndyAyersMS commented Apr 12, 2024

amanasifkhalid commented Apr 12, 2024

amanasifkhalid commented Apr 12, 2024

amanasifkhalid commented Apr 12, 2024

amanasifkhalid commented Apr 11, 2024 •

edited

Loading

VSadov Apr 12, 2024 •

edited

Loading

amanasifkhalid Apr 12, 2024 •

edited

Loading

amanasifkhalid Apr 12, 2024 •

edited

Loading