Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@AndyAyersMS @jakobbotsch @dotnet/jit-contrib PTAL, diffs show no TP regression. I also tried to extend the algorithm to handle nested PHIs, but it didn't find additionally anything. Ideally, like Jakob suggested, we should use |
| } | ||
|
|
||
| //------------------------------------------------------------------------ | ||
| // optGetEdgeAssertions: Given a block and its predecessor, get the assertions |
There was a problem hiding this comment.
I've extracted this function from rangecheck.cpp to re-use in my function
| if (!BitVecOps::MayBeUninit(assertions)) | ||
| { | ||
| AssertionIndex assertionIndex = GetAssertionIndex(index); | ||
| if (assertionIndex > optAssertionCount) |
There was a problem hiding this comment.
redundant condition, I guess it wasn't deleted when we moved all "walk asserts" pieces to BitVecOps::Iter.
src/coreclr/jit/assertionprop.cpp
Outdated
| return blockPred->bbAssertionOut; | ||
| } | ||
|
|
||
| if ((blockPred->KindIs(BBJ_ALWAYS) && blockPred->TargetIs(block)) || |
There was a problem hiding this comment.
I thought for BBJ_ALWAYS the right set was bbAssertionOut?
There was a problem hiding this comment.
Actually for all other BBJ_KINDs you can use bbAssertionOut. Just BBJ_COND is special.
There was a problem hiding this comment.
@AndyAyersMS are you sure? e.g. what about BBJ_SWITCH or some EH related blocks? I copied this function from rangecheck
There was a problem hiding this comment.
I've partially addressed your feedback as I was not confident I can just unconditionally return bbAssertionOut for something other than BBJ_ALWAYS/BBJ_COND(false edge).
However, it looks like we don't typically work with anything but these two anyway.
There was a problem hiding this comment.
bbAssertionOut should be set for all blocks -- for most block kinds it is the only thing we have. Only BBJ_COND also has bbJtrueAssertionOut.
src/coreclr/jit/assertionprop.cpp
Outdated
| return bbJtrueAssertionOut[blockPred->bbNum]; | ||
| } | ||
| } | ||
| return BitVecOps::UninitVal(); |
There was a problem hiding this comment.
Seems like this should return an empty BV on failure, though I also think (once the above is restructured to handle all pred block kinds) you should never get here.
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
|
@AndyAyersMS @jakobbotsch anything else? I've addressed your feedback. Diffs slightly improved (due to better way to look for PhiDefs via VN). I plan to use |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
LGTM, just one thing I'm puzzled about.
| { | ||
| if ((blockPred->KindIs(BBJ_COND) && blockPred->TrueTargetIs(block))) | ||
| { | ||
| if (bbJtrueAssertionOut != nullptr) |
There was a problem hiding this comment.
Curious if you ever see this being null.
There was a problem hiding this comment.
I assumed it could in rangecheck if assertprop didn't run, but not sure such combination is possible. From a quick SPMI run it's not null. I'll convert it into an assert in my next PR to avoid spinning CI for it
|
/ba-g "Linux-arm64 is blocked by dotnet/dnceng#4892, Build-analysis didn't recognize some failures as known build issue " |
* main: JIT: fix case where implied subrange assertions can get lost in morph (dotnet#112020) Propose new async API (dotnet#110420) Move the diagnostic print for stack overflow test failure (dotnet#112001) JIT: Fold more nullchecks (dotnet#111985) Baseline more pri1 tests (dotnet#111068)
3 participants
Handles redundant checks in Example 1 in #111980
Some nice diffs