Improve IsKnownConstant #84002
Improve IsKnownConstant #84002
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsIf we fail to detect a constant in Morph - delegate it to late phases like VN/ConstantProp.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/coreclr/jit/valuenum.cpp
Outdated
| bool isKnownConstant = arg0VNP.BothEqual() && vnStore->IsVNConstant(arg0VNP.GetLiberal()); | ||
| ValueNum isCnsVN = vnStore->VNForIntCon(isKnownConstant ? 1 : 0); | ||
| intrinsic->gtVNPair = vnStore->VNPWithExc(ValueNumPair(isCnsVN, isCnsVN), arg0VNPx); |
There was a problem hiding this comment.
Seems the implementation with both liberal and conservative VNs was lost along the way...
| // IsKnownConstant is expected to be folded in Importer/Morph/VN+ConstantProp | ||
| // This path is just in case if VN+ConstantProp didn't fold them for some reason (or e.g. they were disabled) |
There was a problem hiding this comment.
This is "not enough"...
Consider a case where VN assigns this 1 (i. e. the operand is constant). Then CSE CSEs this as a def with some other unrelated uses of the value 1. Then lowering comes along, and we get 0 instead of 1.
Essentially, each subsequent phase can only further "refine" the value of this intrinsic. The tricky part here is of course that by the time we get to lowering, VNs are no longer valid. So this needs a separate "lower IsKnownConstant" phase, that would use the still-valid VNs. Bit yucky.
There was a problem hiding this comment.
I agree, but the current code is correct, isn't it? it justs conservatively assumes that input is not a constant so we end up in the non-constant path.
I agree that it'd be nice to have a separate phase for such transforms somewhere after assert prop with still valid VNs. Maybe I'll add one eventuallly (and move unrollings there e.g.)
There was a problem hiding this comment.
but the current code is correct, isn't it?
No - consider the CSE example. We cannot have this assigned 1 in VN and then lower it to zero.
There was a problem hiding this comment.
but the current code is correct, isn't it?
No - consider the CSE example. We cannot have this assigned
1in VN and then lower it to zero.
Ah, I see what you mean. I'll implement it in a separate phase if we find a real use case for it
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
If we fail to detect a constant in Morph - delegate it to late phases like VN/ConstantProp.
It allows to use IsKnownConstant for e.g. spans