-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: Remove BBF_FUNCLET_BEG
#113975
base: main
Are you sure you want to change the base?
JIT: Remove BBF_FUNCLET_BEG
#113975
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (17)
- src/coreclr/jit/block.cpp: Language not supported
- src/coreclr/jit/block.h: Language not supported
- src/coreclr/jit/codegenarm.cpp: Language not supported
- src/coreclr/jit/codegenarm64.cpp: Language not supported
- src/coreclr/jit/codegencommon.cpp: Language not supported
- src/coreclr/jit/codegenlinear.cpp: Language not supported
- src/coreclr/jit/codegenloongarch64.cpp: Language not supported
- src/coreclr/jit/codegenriscv64.cpp: Language not supported
- src/coreclr/jit/codegenxarch.cpp: Language not supported
- src/coreclr/jit/compiler.h: Language not supported
- src/coreclr/jit/compiler.hpp: Language not supported
- src/coreclr/jit/emit.cpp: Language not supported
- src/coreclr/jit/fgbasic.cpp: Language not supported
- src/coreclr/jit/fgdiagnostic.cpp: Language not supported
- src/coreclr/jit/fgopt.cpp: Language not supported
- src/coreclr/jit/jiteh.cpp: Language not supported
- src/coreclr/jit/scopeinfo.cpp: Language not supported
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@dotnet/jit-contrib PTAL. No diffs from the last run. I had to make a few changes that seem gratuitous to keep MinOpts TP from regressing. Thanks! |
src/coreclr/jit/codegenlinear.cpp
Outdated
@@ -372,9 +370,10 @@ void CodeGen::genCodeForBBlist() | |||
|
|||
bool firstMapping = true; | |||
|
|||
if (block->HasFlag(BBF_FUNCLET_BEG)) | |||
const bool isFuncletBeg = compiler->UsesFunclets() && compiler->bbIsHandlerBeg(block); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const bool isFuncletBeg = compiler->UsesFunclets() && compiler->bbIsHandlerBeg(block); | |
const bool isFuncletBeg = compiler->bbIsFuncletBeg(block); |
src/coreclr/jit/jiteh.cpp
Outdated
// | ||
bool Compiler::bbIsFuncletBeg(const BasicBlock* block) | ||
{ | ||
if (UsesFunclets() && fgFuncletsCreated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we even be asking this if fgFuncletsCreated
is false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps rearrange so we assert(fgFuncletsCreated);
under if(UsesFunclets())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I started working on this, we had some paths where we would check for funclet start blocks before and after funclets were created, like in fgCanCompactBlock
. I've since been able to remove these checks, so we can make the fgFuncletsCreated
check an invariant.
BBF_FUNCLET_BEG
is set for the first block in each filter/handler region when we have funclets; this state can already be checked withCompiler::bbIsHandlerBeg
. This PR replacesBBF_FUNCLET_BEG
with a new helper methodCompiler::bbIsFuncletBeg
, which is identical in behavior tobbIsHandlerBeg
when we have funclets, and (what should be) a const-evaluatedreturn false
when we don't use funclets at all.This was a no-diff change locally, though it does enable compaction of the first block in a handler region -- I don't see why such a transformation should be illegal.