JIT: one-sided inference from unrelated predicates #72979
Conversation
dotnet-issue-labeler
bot
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.
|
| @@ -178,15 +178,123 @@ void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii) | |||
|
|
|||
| 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 |
| @@ -370,11 +481,26 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) | |||
| return false; | |||
| } | |||
|
|
|||
| // 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). |
|
Superpmi failure is a timeout. |
ghost
locked as resolved and limited conversation to collaborators
Simplistic version where we just look for identical operands.