Skip to content

Commit

Permalink
Fix JitOptRepeat (dotnet#94250)
Browse files Browse the repository at this point in the history
1. Remove an assertion checking assert that can be invalid under JitOptRepeat, where we might
lose information that a constant was ever a handle.
2. Disable JIT/Directed/debugging/debuginfo/tester.csproj under OptRepeat: optimizations
mess with its debug info expectations.
3. Enable JitOptRepeat under stress
  • Loading branch information
BruceForstall authored and matouskozak committed Apr 30, 2024
1 parent 9e43cc0 commit 95acb87
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 2 deletions.
27 changes: 26 additions & 1 deletion src/coreclr/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,13 @@ void Compiler::optPrintAssertion(AssertionDsc* curAssertion, AssertionIndex asse
printf("Exact Type MT(0x%p %s)", dspPtr(iconVal),
eeGetClassName((CORINFO_CLASS_HANDLE)iconVal));
}

// We might want to assert:
// assert(curAssertion->op2.HasIconFlag());
// However, if we run CSE with shared constant mode, we may end up with an expression instead
// of the original handle value. If we then use JitOptRepeat to re-build value numbers, we lose
// knowledge that the constant was ever a handle, as the expression creating the original value
// was not (and can't be) assigned a handle flag.
}
else if (curAssertion->op1.kind == O1K_SUBTYPE)
{
Expand Down Expand Up @@ -1876,7 +1883,6 @@ void Compiler::optDebugCheckAssertion(AssertionDsc* assertion)
{
case O1K_EXACT_TYPE:
case O1K_SUBTYPE:
assert(assertion->op2.HasIconFlag());
break;
case O1K_LCLVAR:
assert((lvaGetDesc(assertion->op1.lcl.lclNum)->lvType != TYP_REF) ||
Expand Down Expand Up @@ -5428,6 +5434,25 @@ GenTree* Compiler::optAssertionProp_Update(GenTree* newTree, GenTree* tree, Stat
if (parent != nullptr)
{
parent->ReplaceOperand(useEdge, newTree);

// If the parent is a GT_IND and we replaced the child with a handle constant, we might need
// to mark the GT_IND as invariant. This is the same as what gtNewIndOfIconHandleNode() does.
// Review: should some kind of more general morphing take care of this?
// Should this share code with gtNewIndOfIconHandleNode()?

if (parent->OperIs(GT_IND) && newTree->IsIconHandle())
{
GenTreeFlags iconFlags = newTree->GetIconHandleFlag();
if (GenTree::HandleKindDataIsInvariant(iconFlags))
{
parent->gtFlags |= GTF_IND_INVARIANT;
if (iconFlags == GTF_ICON_STR_HDL)
{
// String literals are never null
parent->gtFlags |= GTF_IND_NONNULL;
}
}
}
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/jitconfigvalues.h
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ CONFIG_STRING(JitEnableInductionVariableOptsRange, W("JitEnableInductionVariable
CONFIG_INTEGER(JitDoSsa, W("JitDoSsa"), 1) // Perform Static Single Assignment (SSA) numbering on the variables
CONFIG_INTEGER(JitDoValueNumber, W("JitDoValueNumber"), 1) // Perform value numbering on method expressions

CONFIG_INTEGER(JitEnableOptRepeat, W("JitEnableOptRepeat"), 0) // If zero, do not allow JitOptRepeat
CONFIG_INTEGER(JitEnableOptRepeat, W("JitEnableOptRepeat"), 1) // If zero, do not allow JitOptRepeat
CONFIG_METHODSET(JitOptRepeat, W("JitOptRepeat")) // Runs optimizer multiple times on specified methods
CONFIG_INTEGER(JitOptRepeatCount, W("JitOptRepeatCount"), 2) // Number of times to repeat opts when repeating
CONFIG_STRING(JitOptRepeatRange, W("JitOptRepeatRange")) // Enable JitOptRepeat based on method hash range
Expand Down
1 change: 1 addition & 0 deletions src/tests/JIT/Directed/debugging/debuginfo/tester.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<CLRTestEnvironmentVariable Include="DOTNET_JitNoForwardSub" Value="1" />
<CLRTestEnvironmentVariable Include="DOTNET_JitEnableHeadTailMerge" Value="0" />
<CLRTestEnvironmentVariable Include="DOTNET_JitEnableCrossBlockLocalAssertionProp" Value="0" />
<CLRTestEnvironmentVariable Include="DOTNET_JitEnableOptRepeat" Value="0" />

<ProjectReference Include="tests_d.ilproj" Aliases="tests_d" />
<ProjectReference Include="tests_r.ilproj" Aliases="tests_r" />
Expand Down

0 comments on commit 95acb87

Please sign in to comment.