JIT: split loop headers that are also try entries #110880

AndyAyersMS · 2024-12-20T19:09:04Z

when there are backedges that have exited the try. This allows the preheader and header to be in the same EH region and may unblock other optimizations.

This is done by splitting the header block and making the trailing portion be the try entry. Perform a mid (or end) block split if the initial (or all) statements in the header cannot throw.

Fixes #96887.

Enable cloning of loops with EH.

when there are backedges that have exited the try. This allows the preheader and header to be in the same EH region and may unblock other optimizations. This is done by splitting the header block and making the trailing portion be the try entry. Perform a mid (or end) block split if the initial (or all) statements in the header cannot throw. Fixes dotnet#96887. Enable cloning of loops with EH.

AndyAyersMS · 2025-01-06T15:45:25Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

I don't remember what the diffs look like, and the old PR runs are gone, so will update with a summary once the new runs are in.

jakobbotsch · 2025-01-06T15:59:18Z

src/coreclr/jit/optimizer.cpp

+    // If we split any headers we must rebuild DFS/loops.
+    // This should be relatively uncommon.
+    //
+    if (splitHeaders)
+    {
+        fgInvalidateDfsTree();
+        m_dfsTree = fgComputeDfs();
+        m_loops   = FlowGraphNaturalLoops::Find(m_dfsTree);
    }


I'm curious why this is needed when we don't need it in the case where we create new preheaders. It seems like this should not be strictly required for similar reasons as the comment below.

Hmm, I suppose the issue is that splitting a header can change which blocks are the exiting ones.

I wonder if this could be a separate canonicalization step that happens after exit canonicalization, and that then splits the headers for all loops where the header is a try-begin.

In earlier versions I didn't have this bit and ran into issues. Let me look into moving it later.

Maybe what we can do is (1) always create preheader in the same EH region as the header, then (2) canoncalize exits, then (3) split headers where the header is a try entry and not all backedges sources are within the try.

In (1) we can keep track of the loops that require (3).

jakobbotsch · 2025-01-06T16:18:42Z

Does this allow us to assert in fgDebugCheckLoops that no loop-header is a try-begin?

AndyAyersMS · 2025-01-06T16:21:08Z

Does this allow us to assert in fgDebugCheckLoops that no loop-header is a try-begin?

Seems like it should -- either all backedges are from within the try, in which case the preheader becomes the new try entry, or some backedges are from an outer region, in which case the try entry is pushed down inside the loop.

jakobbotsch · 2025-01-06T16:28:53Z

Does this allow us to assert in fgDebugCheckLoops that no loop-header is a try-begin?

Seems like it should -- either all backedges are from within the try, in which case the preheader becomes the new try entry, or some backedges are from an outer region, in which case the try entry is pushed down inside the loop.

Can you also add a small note to

runtime/src/coreclr/jit/compiler.h

Lines 2063 to 2065 in 5824c47

    
           // * All loop blocks are dominated by the header block, i.e. the header block 
        
           //   is guaranteed to be entered on every iteration. Note that in the prescence 
        
           //   of exceptional flow the header might not fully execute on every iteration.

? I believe we have a stronger guarantee after this canonicalization has happened, namely that the header is always exited, i.e. the same as the normal dominance relation we get from the dominator tree.

I also think this comment can be deleted (and the loop below potentially simplified, as the ContainsBlock check should be always true now, but I'm ok with leaving the code alone):

runtime/src/coreclr/jit/optimizer.cpp

Lines 3970 to 3978 in 5824c47

    
           // Note that there is a mismatch between the dominator tree dominance 
        
           // and loop header dominance; the dominator tree dominance relation 
        
           // guarantees that a block A that dominates B was exited before B is 
        
           // entered, meaning it could not possibly have thrown an exception. On 
        
           // the other hand loop finding guarantees only that the header was 
        
           // entered before other blocks in the loop. If the header is a 
        
           // try-begin then blocks inside the catch may not necessarily be fully 
        
           // dominated by the header, but may still be part of the loop. 
        
           //

AndyAyersMS · 2025-01-06T17:47:44Z

Current diffs -- looks like we are missing some collections.

AndyAyersMS · 2025-01-07T16:21:34Z

? I believe we have a stronger guarantee after this canonicalization has happened, namely that the header is always exited, i.e. the same as the normal dominance relation we get from the dominator tree.

Not sure about this one, unless you are proposing we always split the header into an exception-free portion (possibly an empty block) and exception-possible portion. Currently we'd only do this split if there are backedges from sources that are not within the try.

What is true after this is that no block in the loop is dominated by a block outside the try. Either the try entry is the preheader (so the preheader dominates), or the try entry is inside the loop (so the loop header dominates).

AndyAyersMS · 2025-01-07T21:09:24Z

I moved the header split to after exit edge processing. To make this work I needed to know the preheader and the simplest route seemed to be to update the entry edge list.

I haven't done anything with the dominance guarantees; if we want to pursue this then we can change the "split if necessary" to always split loop headers in try regions (or perhaps just try regions whose handlers may resume in the loop). I suspect the empty header block that will sometimes be needed to provide this guarantee will often get merged into its successor, so in those cases the guarantee would be fragile.

jakobbotsch · 2025-01-07T22:23:02Z

src/coreclr/jit/optimizer.cpp

+        while ((splitBefore != stopStmt) && (splitBefore->GetRootNode()->gtFlags & GTF_EXCEPT) == 0)
+        {
+            splitBefore = splitBefore->GetNextStmt();
+        }


Does this need to check for GTF_CALL too? I think your recent try-removal passes did that to be safe.

Since we're post-morph I would hope GTF_EXCEPT is sufficient -- those other phases run very early where we're not yet doing flags validation.

I'm mostly worried that certain classes of exceptions that we sometimes allow reordering (like OutOfMemoryException) would suddenly end up being moved out of a try-region. There may be some cases where we intentionally set only GTF_CALL but not GTF_EXCEPT, but I'm not sure.

It does look like allocation helpers are considered "no throw" and only get GTF_CALL, so we probably do need to check for that here too. Seems like we are moving to a state where we need to start differentiating between "throws reorderable exceptions" and "truly does not throw", since only the latter can be moved out of a try region.

Ok, added GTF_CALL.

jakobbotsch · 2025-01-07T22:28:16Z

src/coreclr/jit/optimizer.cpp

+    BasicBlock* header    = loop->GetHeader();
+    BasicBlock* preheader = loop->GetPreheader();
+
+    if (BasicBlock::sameTryRegion(header, preheader))


Would it be sufficient to check bbIsTryBeg directly here? Then we could avoid having to look for the preheader / the entry edge updating.

It would but then I'd have to do a bit of work to find the proper EH region for the header (not a big deal, we already looked for it when we created the preheader).

jakobbotsch

Looks great to me, thanks for doing this.

jakobbotsch · 2025-01-07T22:31:19Z

Not sure about this one, unless you are proposing we always split the header into an exception-free portion (possibly an empty block) and exception-possible portion. Currently we'd only do this split if there are backedges from sources that are not within the try.

What is true after this is that no block in the loop is dominated by a block outside the try. Either the try entry is the preheader (so the preheader dominates), or the try entry is inside the loop (so the loop header dominates).

What I mean is: does this canonicalization make it so that all loop blocks are dominated by the loop header (for the traditional definition of dominance)? I think it does: if the header cannot be a try-begin anymore, then throwing an exception in the header will always exit the loop. Thus any time a loop block is entered, we know that we executed all of the header.

jakobbotsch · 2025-01-07T22:35:01Z

That's probably not true actually, due to the special case where handlers are allowed to reenter their corresponding try-region at any point, not just at the beginning. So you could have odd loop shapes that start in the middle of a try region, throw an exception to enter the handler, and then jump back into the middle of the try region to continue the loop.

AndyAyersMS · 2025-01-07T22:39:50Z

Right, if the loop is inside the try and the try's catch can resume at the loop header then you can iterate the loop without fully executing the header (though possibly these all end up looking like irreducible cases...?)

AndyAyersMS · 2025-01-08T15:59:59Z

spmi failure is a missing mch ...

AndyAyersMS · 2025-01-08T16:00:05Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2025-01-08T16:00:29Z

Azure Pipelines successfully started running 1 pipeline(s).

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

dotnet-policy-service bot assigned AndyAyersMS Dec 20, 2024

build-analysis bot mentioned this pull request Dec 20, 2024

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

3 tasks

AndyAyersMS added 3 commits December 21, 2024 14:29

need to consistently use the same traits

e492001

avoid odd x86 case; fix if conversion dumping

43b14ad

Merge branch 'main' into TryLoopDisambiguation

eeb6710

AndyAyersMS marked this pull request as ready for review January 6, 2025 15:43

AndyAyersMS requested a review from jakobbotsch January 6, 2025 15:43

jakobbotsch reviewed Jan 6, 2025

View reviewed changes

AndyAyersMS added 2 commits January 7, 2025 11:06

split headers after exit canonicalization

bfe577d

add diagnostic assert that loop headers are not try entries

923e0e6

jakobbotsch reviewed Jan 7, 2025

View reviewed changes

jakobbotsch approved these changes Jan 7, 2025

View reviewed changes

more cautious header splitting

3948da8

AndyAyersMS merged commit 5f3aa0c into dotnet:main Jan 8, 2025
124 of 126 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: split loop headers that are also try entries #110880

JIT: split loop headers that are also try entries #110880

AndyAyersMS commented Dec 20, 2024

AndyAyersMS commented Jan 6, 2025

jakobbotsch Jan 6, 2025

jakobbotsch Jan 6, 2025 •

edited

Loading

AndyAyersMS Jan 6, 2025

AndyAyersMS Jan 7, 2025

jakobbotsch commented Jan 6, 2025

AndyAyersMS commented Jan 6, 2025

jakobbotsch commented Jan 6, 2025

AndyAyersMS commented Jan 6, 2025

AndyAyersMS commented Jan 7, 2025 •

edited

Loading

AndyAyersMS commented Jan 7, 2025 •

edited

Loading

jakobbotsch Jan 7, 2025

AndyAyersMS Jan 7, 2025

jakobbotsch Jan 7, 2025 •

edited

Loading

jakobbotsch Jan 7, 2025

AndyAyersMS Jan 8, 2025

jakobbotsch Jan 7, 2025

AndyAyersMS Jan 7, 2025

jakobbotsch left a comment

jakobbotsch commented Jan 7, 2025

jakobbotsch commented Jan 7, 2025

AndyAyersMS commented Jan 7, 2025

AndyAyersMS commented Jan 8, 2025

AndyAyersMS commented Jan 8, 2025

azure-pipelines bot commented Jan 8, 2025

JIT: split loop headers that are also try entries #110880

JIT: split loop headers that are also try entries #110880

Conversation

AndyAyersMS commented Dec 20, 2024

AndyAyersMS commented Jan 6, 2025

Choose a reason for hiding this comment

jakobbotsch Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Jan 6, 2025

AndyAyersMS commented Jan 6, 2025

jakobbotsch commented Jan 6, 2025

AndyAyersMS commented Jan 6, 2025

AndyAyersMS commented Jan 7, 2025 • edited Loading

AndyAyersMS commented Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Jan 7, 2025 • 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

jakobbotsch left a comment

Choose a reason for hiding this comment

jakobbotsch commented Jan 7, 2025

jakobbotsch commented Jan 7, 2025

AndyAyersMS commented Jan 7, 2025

AndyAyersMS commented Jan 8, 2025

AndyAyersMS commented Jan 8, 2025

azure-pipelines bot commented Jan 8, 2025

jakobbotsch Jan 6, 2025 •

edited

Loading

AndyAyersMS commented Jan 7, 2025 •

edited

Loading

AndyAyersMS commented Jan 7, 2025 •

edited

Loading

jakobbotsch Jan 7, 2025 •

edited

Loading