Skip to content

Fix JitOptRepeat #94250

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 1, 2024
Merged

Fix JitOptRepeat #94250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
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());
Copy link
Member

@EgorBo EgorBo Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't create such assertions in the first places rather than relaxing this requirement.

Copy link
Member

@AndyAyersMS AndyAyersMS Mar 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be ok taking this and addressing that in a follow-up?

Copy link
Member

@EgorBo EgorBo Mar 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok to me to merge if it's blocking, my concern that we're effectively allowing arbitary constants for these assertions - from a quick look, users of O1K_EXACT_TYPE/O1K_SUBTYPE assume it's always a valid handle we can pass to VM, etc. (I guess they need to use HasIconFlag too then)

Another concern that we try to avoid generating not-useful assertions since we run out of assertion limit farily often

Copy link
Contributor Author

@BruceForstall BruceForstall Mar 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can generate an assertion for:

N010 ( 18, 25) [000186] -A---O-----                         *  JTRUE     void   $VN.Void
N009 ( 16, 23) [000031] JA---O-N---                         \--*  EQ        int    $282
N002 (  3,  2) [000030] #----O-----                            +--*  IND       long   $103
N001 (  1,  1) [000029] -----------                            |  \--*  LCL_VAR   ref    V02 arg2         u:1 $82
N008 ( 12, 20) [000202] -A---------                            \--*  COMMA     long   $1c5
N004 (  7, 15) [000198] DA---------                               +--*  STORE_LCL_VAR long   V14 tmp10        d:1 $VN.Void
N003 (  3, 12) [000197] -----------                               |  \--*  CNS_INT   long   0x1237f0098 $1c3
N007 (  5,  5) [000201] -------N---                               \--*  ADD       long   $1c5
N005 (  3,  2) [000199] -----------                                  +--*  LCL_VAR   long   V14 tmp10        u:1 $1c3
N006 (  1,  2) [000200] -----------                                  \--*  CNS_INT   long   0x768 $1c4

namely,

GenTreeNode creates assertion:
N010 ( 18, 25) [000186] -A---O-----                         *  JTRUE     void   $VN.Void
In BB04 New Global Type     Assertion: ($82,$1c5) V02.01 is Exact Type MT(0x0x1237f0800 <unknown class>)

The actual number is correct, but there is no handle data (it's been lost with shared constant CSE).

Where in the code could this be wrong? I.e., where would we look at this value, pass it the VM, etc.?

I guess in optPrintAssertion we cast the constant to CORINFO_CLASS_HANDLE (for non-R2R/NAOT) and pass it to eeGetClassName. But it would be the same number as when we knew it was a handle.

(The previous iteration, the tree looked like:

N005 (  9, 17) [000186] -----O-----                         *  JTRUE     void
N004 (  7, 15) [000031] J----O-N---                         \--*  EQ        int
N002 (  3,  2) [000030] #----O-----                            +--*  IND       long
N001 (  1,  1) [000029] -----------                            |  \--*  LCL_VAR   ref    V02 arg2         u:1
N003 (  3, 12) [000028] H----------                            \--*  CNS_INT(h) long   0x1237f0800 class <unknown class>

)

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
View file
Open in desktop
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
View file
Open in desktop
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