Disable HANDLER_ENTRY_MUST_BE_IN_HOT_SECTION #71273
Conversation
ghost
assigned 
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsFollow-up to #71236.
|
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Disabling @AndyAyersMS PTAL. Assuming this solution is acceptable, would you like me to remove the macro from the codebase, or just leave it disabled for now? Thanks! |
src/coreclr/jit/fgbasic.cpp
Outdated
| // (we want to enable fgCloneFinally() optimization). | ||
| if (hndBegBB->bbJumpKind != BBJ_EHFINALLYRET) | ||
| { | ||
| hndBegBB->bbSetRunRarely(); |
There was a problem hiding this comment.
Why do we make any sort of assumption here?
Seems like perhaps we should just leave hndBegBB alone in this case.
There was a problem hiding this comment.
As far as I can tell, this assumption may be useful for selecting a splitting point in fgDetermineFirstColdBlock, but this hasn't been observed since HANDLER_ENTRY_MUST_BE_IN_HOT_SECTION has been forcing handler entries to be hot. Without this assumption, I don't think a handler entry will ever be selected as a candidate for fgFirstColdBlock here.
Are we ok with keeping this limitation, where handler entries cannot be cold? If so, I can clean this up.
There was a problem hiding this comment.
Seems like perhaps we should just leave
hndBegBBalone in this case.
@AndyAyersMS Do you think that here, and for the filtBB->bbSetRunRarely() case below, if we're not forcing the weight, we should just leave it alone and let natural weights be applied? In particular, we don't want to overwrite profile collected weights, which this is doing. It seems like the right thing to do would be to add logic to fgComputeMissingBlockWeights(), which computes weights for blocks that don't have any profile weights, so for every handler we call bbSetRunRarely() there.
There was a problem hiding this comment.
Something like that.
In the absence of PGO, I think we can just decide catch / filter / fault funclets are rare with impunity (since they are only reached if there is an exception), but finallys should have a weight similar to the callfinallys that can invoke them.
|
Note that this change shows a lot of asm diffs |
|
I've moved the logic for setting handler entry weights to
|
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
To minimize asmdiffs, the above changes in |
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
LGTM.
You can leave removal of HANDLER_ENTRY_MUST_BE_IN_HOT_SECTION for a follow-up change. @BruceForstall said he might dig into the old change history and figure out when/why this was added.
Follow-up to #71236.
HANDLER_ENTRY_MUST_BE_IN_HOT_SECTIONis currently hard-coded to1, but the EE currently handles cold handler entrypoints correctly. This PR experiments with disablingHANDLER_ENTRY_MUST_BE_IN_HOT_SECTION. If successful, this could simplify code logic and enable hot/cold splitting in more scenarios.