JIT: run a late pass of empty try/finally/fault removal

[AndyAyersMS]

The JIT can sometimes optimize away all the code in a finally or fault, so rerun and generalize the empty try-finally removal to cover faults and run it again after the main optimization passes.

[dotnet-policy-service]

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

[AndyAyersMS]

@BruceForstall PTAL
cc @dotnet/jit-contrib

Expect modest diffs (including some regressions where LSRA has made different choices).

[EgorBo]

I presume by the time useProfile is true this assert should be always true? not sure it makes sense to keep, though

[AndyAyersMS]

Looks like there's a widespread issue. The AddCodeDsc records capture EH region indices and these need to be updated when an EH region is removed.

[AndyAyersMS]

Seems like fixing this is not so simple. There are several issues here. I'll use ACD as a shorthand for AddCodeDesc.

• the ACD list "data" entries must be updated in fgRemoveEHTableEntry since EH region indices are shifting
• the map from (kind, data) -> ACD likewise needs updates. Note because we can remove try regions we may discover that there are now redundant ACD entries (for example if a try and its enclosing region both need range checks) and so we need to prune. Redundancy detection is a bit tricky since the other ACDs may also be changing their data.
• the ACD basic block references may become stale; in particular they may refer to blocks that are no longer in the flow graph (say from block compaction). Thus when we update the EH indexes for blocks in the flow graph the block indices referred to by ACD may not be updated.

This latter problem exists even without a late EH opts pass, but things work today because fgNewBBInRegion tolerates being passed blocks no longer in the flow graph so long as the block EH info is still representative of the right region.

To avoid the stale block references it probably makes sense for the ACD entries to record the try and handler indices (which they sort of do now, encoded in data) and simply rely on those, then adjust those as needed if EH regions are removed later, and use them to direct the throw block placement.

That seems like a preqrequisite change, so I may work on it first and come back to this.

[BruceForstall]

fyi, I've wondered whether we can stop setting BBF_DONT_REMOVE on EH blocks. It seems like there would be better, more precise, ways of keeping EH around if needed. #82336

[BruceForstall]

Should PHASE_EMPTY_TRY also be run again?

I'm surprised that moving funclet creation didn't cause any problems (or maybe the problems are hidden behind the ACD failures)?

[AndyAyersMS]

Should PHASE_EMPTY_TRY also be run again?

I'm surprised that moving funclet creation didn't cause any problems (or maybe the problems are hidden behind the ACD failures)?

Yeah it might be worth rerunning that too.

I don't have a complete fix for the ACD issues yet so can't tell if there are other problems. Going to refactor how ACD works first so it is more amenable to EH region removal.

[AndyAyersMS]

I have finally got a version that seems to properly update the ACDs and builds upon the other recent refactorings.

SPMI diffs show modest improvements.

I experimented with running this just after morph as well, and it doesn't add much. I have not yet looked into re-running empty try removal. That seems less likely to kick in? Though perhaps if we go by lack of GTF_EXCEPT, we can remove trys even for non-empty cases. Will add this to my things to look into some day.

I could probably be talked into adding index -> eh region number translation goo for ACDs like we have on basic blocks.

[AndyAyersMS]

Looks like some issues on x86 still.

[AndyAyersMS]

Looks like some issues on x86 still.

Forgot that non-funclet EH only ever uses KD_NONE or KD_TRY (which encode to the same key bits).

[AndyAyersMS]

@BruceForstall ping -- if you're tied up with other work, let me know.

[BruceForstall]

Suggested change

	// true if the key desinator changes
	// true if the key designator changes

[AndyAyersMS]

Will fix subsequently.

[BruceForstall]

Will other flow graph modifications want to do ACD modifications, warranting creating an "API" for doing so (and extracting some of the code here to helper functions)?

[AndyAyersMS]

Will other flow graph modifications want to do ACD modifications, warranting creating an "API" for doing so (and extracting some of the code here to helper functions)?

I'm not aware of any other cases. The ACD's act like virtual blocks that are tied to their EH regions; the only time we change the EH regions of blocks are when we remove (or I suppose add) EH regions, and removal always goes down this path, and adding only happens today for very limited cases before we've ever created any ACDs.