JIT: one-sided inference from unrelated predicates#72979
JIT: one-sided inference from unrelated predicates#72979AndyAyersMS merged 9 commits intodotnet:mainfrom
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsSimplistic version where we just look for identical operands.
|
|
/azp run fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Simplistic version where we just look for identical operands.
|
Looking at regressions I realized that we sometimes stop looking too early. In earlier versions of |
|
Also, a fair number of the larger improvements are from removing what appear to be cloned loops. |
AndyAyersMS
force-pushed
the
RelopImpliesRelopV2
branch
from
9c702bf to
9e5e422
Compare
That's a big win. Just pasting the diff here where we were cloning the loop for redundant condition.
|
|
|
||
| genTreeOps const oper = genTreeOps(funcApp.m_func); | ||
|
|
||
| // Exclude floating point relops. |
There was a problem hiding this comment.
// Note we could also infer the tree relop's value from other
// dominating relops, for example, (x >= 0) dominating (x > 0).
// That is left as a future enhancement.
This comment at the top of function can be deleted now?
| rii->reverseSense = ((treeOper == GT_GT) || (treeOper == GT_LT)); | ||
| rii->canInferFromTrue = true; | ||
| rii->canInferFromFalse = false; | ||
| rii->vnRelation = ValueNumStore::VN_RELATION_KIND::VRK_Unrelated; |
There was a problem hiding this comment.
I am trying to understand the meaning of VRK_Unrelated here. What we are saying is although we can infer R* from R, they are not related meaning they cannot be simply swapped or reversed, but they have slightly different operators. Can we have something meaningful enum value for such cases like VRK_DomInferred or similar?
There was a problem hiding this comment.
Happy to rename it if you think it makes things clearer.
|
@dotnet/jit-contrib PTAL |
|
|
||
| // Was this an inference from an unrelated relop (GE => GT, say)? | ||
| // | ||
| const bool domIsUnrelatedRelop = (rii.vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_Unrelated); |
There was a problem hiding this comment.
likewise here...if you can rename from domIsUnrelatedRelop to domIsInferredRelop, that would be great.
|
LGTM with few suggestions. |
| rii->canInferFromFalse = false; | ||
| rii->canInferFromTrue = true; | ||
| rii->vnRelation = ValueNumStore::VN_RELATION_KIND::VRK_Unrelated; | ||
| break; |
There was a problem hiding this comment.
@AndyAyersMS nit: in my attempt to do something very similar I ended up with a sort of a table, like this:
There was a problem hiding this comment.
Yeah, there's probably some refactoring like this that makes sense.
| @@ -178,15 +178,123 @@ void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii) | |||
|
|
|||
| genTreeOps const oper = genTreeOps(funcApp.m_func); | |||
There was a problem hiding this comment.
I assume it doesn't currently handle unsigned comparisons? (e.g. VNF_GT_UN)
There was a problem hiding this comment.
Correct, it does not handle them.
|
Ok, revised per feedback. Same diffs as before (locally). |
kunalspathak
left a comment
kunalspathak
left a comment
There was a problem hiding this comment.
Refactoring looks much better now. Thanks!
|
Superpmi failure is a timeout. |
ghost
locked as resolved and limited conversation to collaborators
4 participants
Simplistic version where we just look for identical operands.