JIT: local assertion prop and redundant zeroing #74967

AndyAyersMS · 2022-09-01T22:26:07Z

In local assertion prop, optimize out stores of zeros to locations that are
already known to be zero. Don't do this for integer locals as it can lead to
excessive loop cloning.

If we succeed in a LCL_FLD copy prop, try and find a knock-on zero prop.

Update VN to handle the struct LCL_FLD = 0 case better.

diffs

In local assertion prop, optimize out stores of zeros to locations that are already known to be zero. Don't do this for integer locals as it can lead to excessive loop cloning. If we succeed in a LCL_FLD copy prop, try and find a knock-on zero prop. Update VN to handle the `struct LCL_FLD = 0` case better.

ghost · 2022-09-01T22:26:21Z

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

Issue Details

In local assertion prop, optimize out stores of zeros to locations that are
already known to be zero. Don't do this for integer locals as it can lead to
excessive loop cloning.

If we succeed in a LCL_FLD copy prop, try and find a knock-on zero prop.

Update VN to handle the struct LCL_FLD = 0 case better.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2022-09-01T22:26:35Z

@dotnet/jit-contrib PTAL

SingleAccretion

We could also enable VN-based propagation of VNF_ZeroObj, seeing as optVNConstantPropCurStmt has access to the user.

src/coreclr/jit/assertionprop.cpp

SingleAccretion · 2022-09-02T15:17:44Z

src/coreclr/jit/assertionprop.cpp

+                        canOptimize = true;
+                    }
+
+                    // LHS is zero. Is RHS known to be zero from prior assertions?


Would running the ZEROOBJ propagation before this work (it seems unfortunate to duplicate constant propagation logic like this)?

Could be, let me try this.

Seems to work out.

SingleAccretion · 2022-09-02T15:22:54Z

src/coreclr/jit/valuenum.cpp

                {
                    // Note that it is possible to see pretty much any kind of type for the local
                    // (not just TYP_STRUCT) here because of the ASG(BLK(ADDR(LCL_VAR/FLD)), 0) form.
                    initObjVN = (lhsVarDsc->TypeGet() == TYP_STRUCT) ? vnStore->VNForZeroObj(lhsVarDsc->GetStructHnd())
                                                                     : vnStore->VNZeroForType(lhsVarDsc->TypeGet());
                }
+                else if (isZeroInit)
+                {
+                    initObjVN = (lhs->TypeGet() == TYP_STRUCT) ? vnStore->VNForZeroObj(layout->GetClassHandle())


Not all layouts will have handles. The right(-ish, it's not actually optimal, but better than the current state) fix for this is to simply replace the first argument of VNF_ZeroObj with a VNForIntPtrCon(reinterpret_cast<ssize_t>(layout)).

I don't understand what you are suggesting here.

To replace VN of CORINFO_CLASS_HANDLE with VN of ClassLayout* as the argument to VNF_ZeroObj.

So, use the layout instance to classify the zeros instead of the handle, and use VNForIntPtrCon to encode the layout instance.

AndyAyersMS · 2022-09-02T20:17:49Z

We could also enable VN-based propagation of VNF_ZeroObj, seeing as optVNConstantPropCurStmt has access to the user.

I wonder if this would lead us (at times) to reconsider what expansion we'd choose.

I think I'll keep this to mainly be a local AP change.

SingleAccretion · 2022-09-02T20:22:32Z

I wonder if this would lead us (at times) to reconsider what expansion we'd choose.

In terms of promotion, this should not affect things, as we decide whether to decompose in global morph.

(In fact, we must decide this before SSA, to mark things DNER appropriately, at least in the current design)

I think I'll keep this to mainly be a local AP change.

Of course; was just noting this as one "obvious" area of possible further improvements.

AndyAyersMS · 2022-09-02T21:58:35Z

Native AOT failure looks similar to #75005 (timeout).

BruceForstall · 2022-09-02T22:10:26Z

SuperPMI replay failures, e.g.

ISSUE: <ASSERT> #217629 D:\a\_work\1\s\src\coreclr\jit\lower.cpp (1089) - Assertion failed 'arg->OperIs(GT_OBJ, GT_FIELD_LIST) || arg->OperIsLocalRead()' in 'Microsoft.CodeAnalysis.VisualBasic.Formatting.NodeBasedFormattingRule:AddIndentBlockOperationsSlow(System.Collections.Generic.List`1[[Microsoft.CodeAnalysis.Formatting.Rules.IndentBlockOperation, Microsoft.CodeAnalysis.Workspaces, Version=4.4.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]],Microsoft.CodeAnalysis.SyntaxNode,byref):this' during 'Lowering nodeinfo' (IL size 1001; hash 0x83392791; FullOpts)

AndyAyersMS · 2022-09-02T22:49:25Z

We should improve the rules for supported IR shapes in the post-phase IR checking. Otherwise, it's a bit of a guessing game.

SingleAccretion · 2022-09-03T10:47:47Z

I see the code which propagates ZEROOBJ on local fields is still there. Likely that's the source of asserts. If we want to propagate zeroes into local fields, it can be done in optZeroObjAssertionProp.

SingleAccretion · 2022-09-03T11:08:45Z

src/coreclr/jit/valuenum.cpp

-                ValueNum initObjVN = ValueNumStore::NoVN;
-                if (isEntire && rhs->IsIntegralConst(0))
+                ValueNum   initObjVN  = ValueNumStore::NoVN;
+                bool const isZeroInit = rhs->IsIntegralConst(0);
+                if (isZeroInit && isEntire)
                {
                    // Note that it is possible to see pretty much any kind of type for the local
                    // (not just TYP_STRUCT) here because of the ASG(BLK(ADDR(LCL_VAR/FLD)), 0) form.
-                    initObjVN = (lhsVarDsc->TypeGet() == TYP_STRUCT) ? vnStore->VNForZeroObj(lhsVarDsc->GetStructHnd())
+                    initObjVN = (lhsVarDsc->TypeGet() == TYP_STRUCT) ? vnStore->VNForZeroObj(lhsVarDsc->GetLayout())
                                                                     : vnStore->VNZeroForType(lhsVarDsc->TypeGet());
                }
+                else if (isZeroInit)
+                {
+                    // Non-entire zeroing
+                    initObjVN = (lhs->TypeGet() == TYP_STRUCT) ? vnStore->VNForZeroObj(layout)
+                                                               : vnStore->VNZeroForType(lhs->TypeGet());
+                }
                else
                {
+                    // Non-zero init, or non-entire block zero init
+                    //
                    // Non-zero block init is very rare so we'll use a simple, unique VN here.
                    initObjVN = vnStore->VNForExpr(compCurBB, lhsVarDsc->TypeGet());
                }


With the layout change, this can be simplified:

ValueNum initObjVN = ValueNumStore::NoVN; if (rhs->IsIntegralConst(0)) { initObjVN = (lhs->TypeGet() == TYP_STRUCT) ? vnStore->VNForZeroObj(layout) : vnStore->VNZeroForType(lhs->TypeGet()); } else { // Non-zero block init is very rare so we'll use a simple, unique VN here. initObjVN = vnStore->VNForExpr(compCurBB, lhs->TypeGet()); }

kunalspathak

Interesting diffs.

kunalspathak · 2022-09-06T16:56:15Z

src/coreclr/jit/assertionprop.cpp

+                // Rescan the table looking for possible constants
+                //
+                onlyConstProp = true;
+                index         = 0;


Probably worth to set the first valid index for which !curAssertion->CanPropLclVar() is true and use that to set the index here to avoid some unnecessary lookups?

Turns out this "second chance" assertion prop no longer kicks in so I just removed it.

Turns out this "second chance" assertion prop no longer kicks in so I just removed it.

Note that morph repeatedly invokes assertion prop so we would naturally have caught this case:

runtime/src/coreclr/jit/morph.cpp

Lines 14069 to 14087 in 3d630a8

/* Before morphing the tree, we try to propagate any active assertions */

if (optLocalAssertionProp)

{

/* Do we have any active assertions? */

if (optAssertionCount > 0)

{

GenTree* newTree = tree;

while (newTree != nullptr)

{

tree = newTree;

/* newTree is non-Null if we propagated an assertion */

newTree = optAssertionProp(apFull, tree, nullptr, nullptr);

}

assert(tree != nullptr);

}

}

PREFAST_ASSUME(tree != nullptr);

}

kunalspathak · 2022-09-06T17:04:19Z

src/coreclr/jit/assertionprop.cpp

+                    // The latter part of the if below is a heuristic.
+                    //
+                    // If we elimiate a zero assignment for integral lclVars it can lead to
+                    // unnecessary cloning. We need to make sure `optExtractInitTestIncr`


it can lead to unnecessary cloning

Not sure if I follow this.

Say we remove a Vxx = 0 because assertion prop already knows Vxx is zero, and Vxx is the control variable for some loop.

With the redundant zeroing removed, optExtractdInitTestIncr may fail to find the initial value for Vxx because it just does a simple local search. When this happens, the cloner will fail to prove that the loop cloning conditions are always satisfied. and so do extra cloning that it otherwise would not do.

What's a little odd here is that we can't then later on (using more powerful VN/global AP) prove that the loop cloning conditions are indeed satisfied and clean things back up as if we'd never cloned. I didn't dig into this because knowingly enabling unnecessary cloning and relying on later phases to clean up seemed like a bad plan.

SingleAccretion · 2022-09-07T16:30:18Z

src/coreclr/jit/assertionprop.cpp

@@ -3821,14 +3840,92 @@ GenTree* Compiler::optAssertionProp_LclFld(ASSERT_VALARG_TP assertions, GenTreeL
 GenTree* Compiler::optAssertionProp_Asg(ASSERT_VALARG_TP assertions, GenTreeOp* asg, Statement* stmt)
 {
    GenTree* rhs = asg->gtGetOp2();
-    if (asg->OperIsCopyBlkOp() && varTypeIsStruct(rhs))


Why the varTypeIsStruct(rhs) condition removal, along with the assert in optZeroObjAssertionProp?

Because optZeroObjAssertionProp was extended to handle LCL_FLD.

AndyAyersMS · 2022-09-07T17:42:05Z

Some kind of CI issue downloading things:

__DistroRid: linux-arm
Downloading 'https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.sh'
curl: (92) HTTP/2 stream 1 was not closed cleanly: CANCEL (err 8)
Curl failed; dumping some information about dotnet.microsoft.com for later investigation

AndyAyersMS · 2022-09-07T22:09:06Z

Retrying the failed legs

AndyAyersMS · 2022-09-08T00:22:06Z

SPMI replay failures are redundant artifacts:

##[error]Artifact SuperPMI_BuildLogs_x64_checked already exists for build 7551.

AndyAyersMS · 2022-09-08T00:24:24Z

Going to assume the small TP regressions are artifacts of the compiler update.

@kunalspathak can you approve since you already are somewhat familiar with the changes?

kunalspathak · 2022-09-08T05:27:45Z

I will take a look tomorrow.

kunalspathak

LGTM

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 1, 2022

ghost assigned AndyAyersMS Sep 1, 2022

SingleAccretion reviewed Sep 2, 2022

View reviewed changes

review feedback

37f67b4

SingleAccretion reviewed Sep 3, 2022

View reviewed changes

kunalspathak reviewed Sep 6, 2022

View reviewed changes

AndyAyersMS added 2 commits September 6, 2022 21:32

more review feedback

4c0ac08

simplify

1a4a403

SingleAccretion reviewed Sep 7, 2022

View reviewed changes

SingleAccretion approved these changes Sep 7, 2022

View reviewed changes

kunalspathak approved these changes Sep 8, 2022

View reviewed changes

AndyAyersMS merged commit 3d630a8 into dotnet:main Sep 8, 2022

ghost locked as resolved and limited conversation to collaborators Oct 9, 2022

	/* Before morphing the tree, we try to propagate any active assertions */
	if (optLocalAssertionProp)
	{
	/* Do we have any active assertions? */

	if (optAssertionCount > 0)
	{
	GenTree* newTree = tree;
	while (newTree != nullptr)
	{
	tree = newTree;
	/* newTree is non-Null if we propagated an assertion */
	newTree = optAssertionProp(apFull, tree, nullptr, nullptr);
	}
	assert(tree != nullptr);
	}
	}
	PREFAST_ASSUME(tree != nullptr);
	}

JIT: local assertion prop and redundant zeroing #74967

JIT: local assertion prop and redundant zeroing #74967

Uh oh!

Conversation

AndyAyersMS commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 1, 2022

Uh oh!

AndyAyersMS commented Sep 1, 2022

Uh oh!

SingleAccretion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Sep 2, 2022

Uh oh!

SingleAccretion commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Sep 2, 2022

Uh oh!

BruceForstall commented Sep 2, 2022

Uh oh!

AndyAyersMS commented Sep 2, 2022

Uh oh!

SingleAccretion commented Sep 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Sep 7, 2022

Uh oh!

AndyAyersMS commented Sep 7, 2022

Uh oh!

AndyAyersMS commented Sep 8, 2022

Uh oh!

AndyAyersMS commented Sep 8, 2022

Uh oh!

kunalspathak commented Sep 8, 2022

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndyAyersMS commented Sep 1, 2022 •

edited

Loading

SingleAccretion Sep 2, 2022 •

edited

Loading

SingleAccretion commented Sep 2, 2022 •

edited

Loading