Use VNVisitReachingVNs in more places #108420
Conversation
ghost
added
the
area-CodeGen-coreclr
…me-1 into use-VNVisitReachingVNs-more
| { | ||
| return optCreateAssertion(tree->AsOp()->gtOp1, nullptr, OAK_NOT_EQUAL); | ||
| } | ||
| return NO_ASSERTION_INDEX; |
There was a problem hiding this comment.
No diff change. Also, it effectively repeats IsKnownNonNull now which also walks phis.
| { | ||
| break; | ||
| } | ||
| FALLTHROUGH; |
There was a problem hiding this comment.
Dynamic blocks are no more. No diffs change.
| optImpliedByCopyAssertion(iterAssertion, curAssertion, activeAssertions); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems a little odd that this has no impact... are we not properly maintaining the vn->assertion maps?
There was a problem hiding this comment.
@AndyAyersMS we do maintain it, but it seems that we never arrive here:
runtime/src/coreclr/jit/assertionprop.cpp
Line 5891 in 81efcad
O2K_LCLVAR_COPY is visitted)
There was a problem hiding this comment.
Ok. I don't understand this aspect of AP very well, so perhaps nothing is wrong, but it still feels like at some point either something broke and we didn't realize, or something rendered all this logic irrelevant (in which case perhaps we can save TP by removing more code elsewhere). Maybe open an issue to investigate?
|
@AndyAyersMS @dotnet/jit-contrib PTAL |
|
|
||
| // Helper function for VNVisitReachingVNs | ||
| template <typename TArgVisitor> | ||
| VNVisit VNVisitReachingVNsWorker(ValueNum vn, TArgVisitor argVisitor) |
There was a problem hiding this comment.
Here I split VNVisitReachingVNs into two functions. In most cases the input is not a phi so we run argVisitor just once - it helps us to avoid creating an on-stack array in VNVisitReachingVNsWorker
Improved JIT TP.
| return ValueNumStore::VNVisit::Abort; | ||
| }; | ||
|
|
||
| if (vnStore->VNVisitReachingVNs(value->gtVNPair.GetConservative(), vnVisitor) == ValueNumStore::VNVisit::Continue) |
There was a problem hiding this comment.
Basically, if we have IND(addr) = value where value is either null or an nongc object - we don't need a write-barrier (taking Phi defs into account now).
Similarly, we can walk phis for addr itelf but that didn't show any diffs.
| optImpliedByCopyAssertion(iterAssertion, curAssertion, activeAssertions); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems a little odd that this has no impact... are we not properly maintaining the vn->assertion maps?
| return false; | ||
| } | ||
|
|
||
| bool ValueNumStore::IsPhiDef(ValueNum vn) const |
2 participants
Closes #104489
Diffs
The main idea to walk Phi nodes in
bool ValueNumStore::IsKnownNonNull(ValueNum vn)so if all phi nodes are not-null, then the phi itself is not null as well. Same for "does need a write barrier" detection.The rest of the changes are unrelated, but they were needed to mitigate TP regression