Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: redundant branch destructure dominating and/or #69291

Conversation

AndyAyersMS
Copy link
Member

If a branch predicate p is dominated by another branch with predicate
AND(p, ..) or OR(p, ...) we may be able to infer the value of p.

This is useful on its own, and should help unblock #62689.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 13, 2022
@ghost ghost assigned AndyAyersMS May 13, 2022
@ghost
Copy link

ghost commented May 13, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

If a branch predicate p is dominated by another branch with predicate
AND(p, ..) or OR(p, ...) we may be able to infer the value of p.

This is useful on its own, and should help unblock #62689.

Author: AndyAyersMS
Assignees: AndyAyersMS
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@jakobbotsch PTAL
@dotnet/jit-contrib FYI

Not sure how correct this is yet, I can't seem to run pri0 tests locally anymore. Small number of SPMI diffs, but many are sizeable.

I am tempted to recast this as a general VN utility where we can ask if knowing the value of a VN implies knowing the value of another VN; there are lots of other inferences we can draw of this kind. But I'm not sure how to structure this without it getting overly complicated.

@jakobbotsch
Copy link
Member

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines 180 to 266
if (!matched && vnStore->IsVNFunc(domCmpNormVN))
{
VNFuncApp funcApp;
if (vnStore->GetVNFunc(domCmpNormVN, &funcApp))
{
genTreeOps const oper = genTreeOps(funcApp.m_func);

if ((oper == GT_EQ) || (oper == GT_NE))
{
ValueNum predVN = funcApp.m_args[0];
ValueNum constantVN = funcApp.m_args[1];

if ((constantVN == vnStore->VNZeroForType(TYP_INT)) && vnStore->IsVNFunc(predVN))
{
VNFuncApp predFuncApp;

if (vnStore->GetVNFunc(predVN, &predFuncApp))
{
genTreeOps const predOper = genTreeOps(predFuncApp.m_func);

// Also perhaps GT_NOT?
//
if ((predOper == GT_AND) || (predOper == GT_OR) || (predOper == GT_NOT))
{
// If dominating compare is AND/OR(p1, p2) and one of
// the p's is related to our predicate....
//
for (unsigned int i = 0; (i < predFuncApp.m_arity) && !matched; i++)
{
ValueNum pVN = predFuncApp.m_args[i];

// Also consider perhaps handling N-Ary cases AND(AND(...), ...) and so on.
//
// Abstractly it would be nice if VN allowed n-ary commutative operators
// even though the IR does not support this.
//
for (auto vnRelation : vnRelations)
{
const ValueNum relatedVN = vnStore->GetRelatedRelop(pVN, vnRelation);

if ((relatedVN != ValueNumStore::NoVN) && (relatedVN == treeNormVN))
{
vnRelationMatch = vnRelation;
matched = true;

// If dom predicate is wrapped in EQ(*,0) then a true dom
// predicate implies a false branch outcome, and vice versa.
//
// And if the dom predicate is GT_NOT we reverse yet again.
//
reverseSense = (oper == GT_EQ) ^ (predOper == GT_NOT);

// We only get partial knowledge in these cases.
//
// AND(p1,p2) = true ==> both p1 and p2 must be true
// AND(p1,p2) = false ==> don't know p1 or p2
// OR(p1,p2) = true ==> don't know p1 or p2
// OR(p1,p2) = false ==> both p1 and p2 must be false
//
if (predOper != GT_NOT)
{
canInferFromFalse = reverseSense ^ (predOper == GT_OR);
canInferFromTrue = reverseSense ^ (predOper == GT_AND);
}

JITDUMP("Inferring predicate value from %s\n",
GenTree::OpName(predOper));
break;
}
}
}
}
}
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am tempted to recast this as a general VN utility where we can ask if knowing the value of a VN implies knowing the value of another VN; there are lots of other inferences we can draw of this kind. But I'm not sure how to structure this without it getting overly complicated.

I think this would be great, and in any case it would be nice to extract this to a function to avoid some of the nesting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am tempted to recast this as a general VN utility where we can ask if knowing the value of a VN implies knowing the value of another VN; there are lots of other inferences we can draw of this kind. But I'm not sure how to structure this without it getting overly complicated.

I think this would be great, and in any case it would be nice to extract this to a function to avoid some of the nesting

Let me get the logic right and then I'll look into refactoring.

@jakobbotsch
Copy link
Member

The Fuzzlyn examples may be easier to use than the failing tests to iron out the issues.

@AndyAyersMS
Copy link
Member Author

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

If a branch predicate `p` is dominated by another branch with predicate
`AND(p, ..)` or `OR(p, ...)` we may be able to infer the value of `p`.

This is useful on its own, and should help unblock dotnet#62689.
@AndyAyersMS AndyAyersMS force-pushed the RedunantBranchOptSeeThroughAndOrCombinedPredicates branch from 0071ece to d0366d6 Compare May 20, 2022 01:51
@AndyAyersMS
Copy link
Member Author

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

@jakobbotsch see if this version reads any better. I could pull functionality into the helper struct but have left it basic for now.

Looking at a few of the larger diffs I realized that the existing version of redundant branch opts has a bug; if the tree VN is a constant than the outcome of the branch it controls is independent of any dominating branch, but we were (previously) using inferencing here. This lead to at least one instance where we made the wrong deduction, 57396 in coreclr_tests. Fixing this keeps a bunch of code around that should never have been deleted. I added a note to the code indicating that checking for the constant case is necessary and not just nice to have.

Seemingly these constant VN relops are somewhat rare and having more than one in the right arrangement even rarer, which is why we haven't seen complaints about this before.

;; bogus deduction with constant VNs (now fixed)

N002 [000032]   CNS_INT   1 => $41 {IntCns 1}
...
Dominator BB01 of BB07 has relop with same liberal VN
N003 (  5,  4) [000008] J------N---                         *  LE        int    $41
N001 (  3,  2) [000006] -----------                         +--*  LCL_VAR   int    V03 loc1         u:2 (last use) $40
N002 (  1,  1) [000007] -----------                         \--*  CNS_INT   int    0 $40
 Redundant compare; current relop:
N003 (  3,  3) [000259] J------N---                         *  GE        int    $41
N001 (  1,  1) [000260] -----------                         +--*  LCL_VAR   int    V04 loc2         u:2 $40
N002 (  1,  1) [000261] -----------                         \--*  CNS_INT   int    -1 $42
Fall through successor BB02 of BB01 reaches, relop must be false

The other large regression 247114 from libraries_tests.pmi also has constant cases but there we ended up getting it right as the dominating predicate was the same predicate (here $42 is 0)

N005 [000047]   CNS_INT   0 => $42 {IntCns 0}
...
Dominator BB210 of BB215 has relop with same liberal VN
N003 (  3,  3) [003642] J------N---                         *  EQ        int    $42
N001 (  1,  1) [000486] -----------                         +--*  LCL_VAR   ref    V19 tmp15        u:2 $13c5
N002 (  1,  1) [003641] -----------                         \--*  CNS_INT   ref    null $VN.Null
 Redundant compare; current relop:
N003 (  3,  3) [003711] J------N---                         *  EQ        int    $42
N001 (  1,  1) [000491] -----------                         +--*  LCL_VAR   ref    V19 tmp15        u:2 $13c5
N002 (  1,  1) [003710] -----------                         \--*  CNS_INT   ref    null $VN.Null

The large regression in this method seems to come from LSRA finding many more single-def cases to spill upfront. Did not try and drill into this further.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll do a last Fuzzlyn run.

@jakobbotsch
Copy link
Member

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member

Wow, great diffs. -1% code size and -3% TP on coreclr_tests, looks like that testout test case really hits this pattern.

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented May 23, 2022

Wow, great diffs. -1% code size and -3% TP on coreclr_tests, looks like that testout test case really hits this pattern.

Turns out those methods were all optimized by the "constant" case. I put "constant" in quotes because we're now looking at the liberal VN for a read of a static that we haven't modified since we set its value and haven't done any in-between heap updates.

We would not optimize these before, eg here's some pre-PR IR post-lower.

N001 [000001]   CNS_INT   1 => $42 {IntCns 1}
N002 [000820]   IND       => <l:$42 {IntCns 1}, c:$500 {MemOpaque:NotInLoop}>
...
------------ BB02 [06B..072) -> BB04 (cond), preds={BB01} succs={BB03,BB04}
N001 (  2, 10) [000939] H----------                  t939 =    CNS_INT(h) long   0x177d7f3d9f1 static Fseq[hackishFieldName] $c9
                                                            /--*  t939   long   
N002 (  5, 13) [000820] n---G------                  t820 = *  IND       bool   <l:$42, c:$500>
N003 (  1,  1) [000821] -----------                  t821 =    CNS_INT   int    0 $40
                                                            /--*  t820   bool   
                                                            +--*  t821   int    
N004 (  7, 15) [000822] J---G--N---                  t822 = *  NE        int    <l:$42, c:$4c2>
                                                            /--*  t822   int    
N005 (  9, 17) [000823] ----G------                         *  JTRUE     void   $VN.Void

@AndyAyersMS
Copy link
Member Author

Also skimmed Fuzzlyn failures; they don't seem to be related?

@jakobbotsch
Copy link
Member

Also skimmed Fuzzlyn failures; they don't seem to be related?

Agreed, all of them look like #69659.

@AndyAyersMS AndyAyersMS merged commit 315c31c into dotnet:main May 23, 2022
@EgorBo
Copy link
Member

EgorBo commented Jun 28, 2022

Nice improvements on x64: dotnet/perf-autofiling-issues#5617

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants