-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: cross-block local assertion prop in morph #94363
Changes from 4 commits
ad42b51
cceb9f8
bc59375
2b78bdc
8fa2318
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13780,10 +13780,80 @@ void Compiler::fgMorphBlock(BasicBlock* block) | |||||
|
||||||
if (optLocalAssertionProp) | ||||||
{ | ||||||
// For now, each block starts with an empty table, and no available assertions | ||||||
// | ||||||
optAssertionReset(0); | ||||||
apLocal = BitVecOps::MakeEmpty(apTraits); | ||||||
if (!optCrossBlockLocalAssertionProp) | ||||||
{ | ||||||
// Each block starts with an empty table, and no available assertions | ||||||
// | ||||||
optAssertionReset(0); | ||||||
apLocal = BitVecOps::MakeEmpty(apTraits); | ||||||
} | ||||||
else | ||||||
{ | ||||||
// Determine if this block can leverage assertions from its pred blocks. | ||||||
// | ||||||
// Some blocks are ineligible. | ||||||
// | ||||||
bool canUsePredAssertions = ((block->bbFlags & BBF_CAN_ADD_PRED) == 0) && !bbIsHandlerBeg(block); | ||||||
|
||||||
// Validate all preds have valid info | ||||||
// | ||||||
if (canUsePredAssertions) | ||||||
{ | ||||||
bool hasPred = false; | ||||||
|
||||||
for (BasicBlock* const pred : block->PredBlocks()) | ||||||
{ | ||||||
// A higher postorder number means the block appears earlier in | ||||||
// the postorder. Use this to detect if a pred's assertion info is available. | ||||||
// | ||||||
if (pred->bbPostorderNum > block->bbPostorderNum) | ||||||
{ | ||||||
// Yes, available. If this is the first pred, copy. | ||||||
// If this is a subsequent pred, intersect. | ||||||
// | ||||||
if (!hasPred) | ||||||
{ | ||||||
apLocal = BitVecOps::MakeCopy(apTraits, pred->bbAssertionOut); | ||||||
hasPred = true; | ||||||
} | ||||||
else | ||||||
{ | ||||||
BitVecOps::IntersectionD(apTraits, apLocal, pred->bbAssertionOut); | ||||||
} | ||||||
|
||||||
continue; | ||||||
} | ||||||
|
||||||
// No, not available. | ||||||
// | ||||||
JITDUMP(FMT_BB " pred " FMT_BB " not processed; clearing assertions in\n", block->bbNum, | ||||||
pred->bbNum); | ||||||
canUsePredAssertions = false; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add Or better, make the failure case first, with |
||||||
} | ||||||
|
||||||
// If there were no preds, there are no asserions in. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// | ||||||
if (!hasPred) | ||||||
{ | ||||||
JITDUMP(FMT_BB " has no preds, so no assertions in\n", block->bbNum); | ||||||
canUsePredAssertions = false; | ||||||
} | ||||||
|
||||||
if (canUsePredAssertions) | ||||||
{ | ||||||
JITDUMPEXEC(optDumpAssertionIndices("Assertions in: ", apLocal)); | ||||||
} | ||||||
} | ||||||
else | ||||||
{ | ||||||
JITDUMP(FMT_BB " ineligible for cross-block; clearing assertions in\n", block->bbNum); | ||||||
} | ||||||
|
||||||
if (!canUsePredAssertions) | ||||||
{ | ||||||
apLocal = BitVecOps::MakeEmpty(apTraits); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this also need Can the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No. There are 3 modes of operation here:
Modes 2 and 3 can mix with each other, but not with mode 1. We use mode 1 if the feature is disabled, or if the method has a lot of locals. When the feature is enabled, and the method does not have a lot of locals, we use mode 2 (if
Let me look at tweaking this. |
||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
// Make the current basic block address available globally. | ||||||
|
@@ -13801,6 +13871,14 @@ void Compiler::fgMorphBlock(BasicBlock* block) | |||||
} | ||||||
} | ||||||
|
||||||
// Publish the live out state. | ||||||
// | ||||||
if (optCrossBlockLocalAssertionProp && (block->NumSucc() > 0)) | ||||||
{ | ||||||
assert(optLocalAssertionProp); | ||||||
block->bbAssertionOut = BitVecOps::MakeCopy(apTraits, apLocal); | ||||||
} | ||||||
|
||||||
compCurBB = nullptr; | ||||||
} | ||||||
|
||||||
|
@@ -13822,7 +13900,8 @@ PhaseStatus Compiler::fgMorphBlocks() | |||||
|
||||||
// Local assertion prop is enabled if we are optimized | ||||||
// | ||||||
optLocalAssertionProp = opts.OptimizationEnabled(); | ||||||
optLocalAssertionProp = opts.OptimizationEnabled(); | ||||||
optCrossBlockLocalAssertionProp = optLocalAssertionProp; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see you consult the config in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went back and forth as to whether all this setup should be assertion prop's business; I could push more of the logic there I suppose. |
||||||
|
||||||
if (optLocalAssertionProp) | ||||||
{ | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,7 @@ | |
RunningIlasmRoundTrip; | ||
DOTNET_JitSynthesizeCounts; | ||
DOTNET_JitCheckSynthesizedCounts | ||
DOTNET_JitDoCrossBlockLocalAssertionProp | ||
</DOTNETVariables> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
|
@@ -220,6 +221,7 @@ | |
<TestEnvironment Include="jitobjectstackallocation" JitObjectStackAllocation="1" TieredCompilation="0" /> | ||
<TestEnvironment Include="jitphysicalpromotion_only" JitStressModeNames="STRESS_NO_OLD_PROMOTION" TieredCompilation="0" /> | ||
<TestEnvironment Include="jitphysicalpromotion_full" JitStressModeNames="STRESS_PHYSICAL_PROMOTION_COST STRESS_NO_OLD_PROMOTION" TieredCompilation="0" /> | ||
<TestEnvironment Include="jitcrossblocklocalassertionprop" JitDoCrossBlockLocalAssertionProp="1" TieredCompilation="0" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
<TestEnvironment Include="jitcfg" JitForceControlFlowGuard="1" /> | ||
<TestEnvironment Include="jitcfg_dispatcher_always" JitForceControlFlowGuard="1" JitCFGUseDispatcher="1" /> | ||
<TestEnvironment Include="jitcfg_dispatcher_never" JitForceControlFlowGuard="1" JitCFGUseDispatcher="0" /> | ||
|
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.
Do
seems fine, but should we useEnable
instead, to match other similar cases (likeJitEnablePhysicalPromotion
, above)?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.
Sure... it was up with the other
Do
cases before, but now that I moved it to be available in release it no longer matches its neighbors.