-
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
Don't use VN when it's already stale #97943
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsRuntime lookup expansion could use VNs when it's no longer legal.
|
A regression example: Details
It looks like |
GenTree** callUse = nullptr; | ||
Statement* newFirstStmt = nullptr; | ||
block = fgSplitBlockBeforeTree(block, stmt, call, &newFirstStmt, &callUse); | ||
assert(prevBb != nullptr && block != 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.
Unrelated change, just a clean up since SplitAtTreeAndReplaceItWithLocal
does exactly that.
PTAL @jakobbotsch @AndyAyersMS cc @dotnet/jit-contrib Diffs 12 methods regressed in Tests because of #97943 (comment) |
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.
LGTM.
Looks like the gcinfo use of VNs goes way back.
CI Failure is #97049 |
@@ -240,8 +240,8 @@ GCInfo::WriteBarrierForm GCInfo::gcIsWriteBarrierCandidate(GenTreeStoreInd* stor | |||
} | |||
|
|||
// Ignore any assignments of NULL. | |||
GenTree* const data = store->Data()->gtSkipReloadOrCopy(); | |||
if ((data->GetVN(VNK_Liberal) == ValueNumStore::VNForNull()) || data->IsIntegralConst(0)) | |||
GenTree* const data = store->Data()->gtSkipReloadOrCopy()->gtEffectiveVal(); |
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.
Why this change? GT_COMMA
should not exist at this point, and if it did it is questionable to not skip reloads inside it (would cause issues during codegen).
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.
Agree, completely unnecessary, will remove as part of some other PR
@@ -4101,7 +4101,7 @@ enum GenTreeCallFlags : unsigned int | |||
GTF_CALL_M_GUARDED_DEVIRT_CHAIN = 0x00080000, // this call is a candidate for chained guarded devirtualization | |||
GTF_CALL_M_ALLOC_SIDE_EFFECTS = 0x00100000, // this is a call to an allocator with side effects | |||
GTF_CALL_M_SUPPRESS_GC_TRANSITION = 0x00200000, // suppress the GC transition (i.e. during a pinvoke) but a separate GC safe point is required. | |||
GTF_CALL_M_EXP_RUNTIME_LOOKUP = 0x00400000, // this call needs to be transformed into CFG for the dynamic dictionary expansion feature. | |||
GTF_CALL_M_EXP_RUNTIME_LOOKUP = 0x00400000, // [DEBUG only] this call needs to be transformed into CFG for the dynamic dictionary expansion feature. |
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.
If this is debug only can you please move it to GenTreeCallDebugFlags
?
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 was sort of unrelated to this PR, but good idea! One less bit occupied.
Runtime lookup expansion could use VNs when it's no longer legal.
gcIsWriteBarrierCandidate
could use VNs even in codegen phase.VN_BASED_DEAD_STORE_REMOVAL
is the last phase where we still can rely on VNs.