-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: consistently mark method table accesses as invariant #45854
Conversation
Introduce a new utility method to create the IR for method table access, marking the resulting indir as invariant. This allows method table access CSEs (which will be increasingly common with the advent of PGO-enabled Guarded Devirtualization). Add a workaround to assertion prop for method table to be able to see through a CSE'd method table access. Remove optimizer inhibitions related to colon cond, as there are no qmark nodes around by this point.
@briansull PTAL Relatively modest impact. Most regressions are extra CSEs. A couple of them are missed knock-on bounds checks caused by extra CSEs (see #45658). Large wins are in methods that have big chains of type tests guarding casts...
|
@@ -2111,11 +2114,6 @@ void Compiler::optAssertionGen(GenTree* tree) | |||
{ | |||
tree->ClearAssertion(); | |||
|
|||
if (tree->gtFlags & GTF_COLON_COND) |
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.
Is GTF_COLON_COND dead code that is being removed with this change?
Maybe change it to an assert here.
I don't believe that we can create an assertion that is predicated on a conditional
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.
We remove QMARKS in morph, so perhaps this is too aggressive. I'll restore the bail out but only do it for local assertion prop.
// original tree, of not. | ||
// | ||
inline GenTree* GenTree::gtUnwrapCSE() | ||
{ |
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.
While this is the format that we use for a CSE def there are several other places we will generate GT_COMMA's with this patterm.
Looking through the code base this function also is commonly used to generate an assignment to a new temp LclVar
GenTree* Compiler::fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE structType /*= nullptr*/)
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.
It doesn't have to be a CSE, it could be any value. I can rename the helper.
During global prop we have a mixture of rules that key off VNs and also look at IR shapes. In general we should just be able to use the VNs to figure most things out, but that doesn't happen consistently today. So in this case I need to find the original IR shape.
@dotnet/jit-contrib ping |
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.
You removed some GTF_EXCEPT bits; I presume you've proved those are unnecessary, despite the comments there saying "null pointer exception"?
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.
Looks Good
The new helper goes through |
Introduce a new utility method to create the IR for method table access,
marking the resulting indir as invariant. This allows method table access
CSEs (which will be increasingly common with the advent of PGO-enabled
Guarded Devirtualization).
Add a workaround to assertion prop for method table to be able to see
through a CSE'd method table access.
Remove optimizer inhibitions related to colon cond, as there are no qmark
nodes around by this point.