JIT: Stop creating "location" commas #83814

jakobbotsch · 2023-03-23T12:53:52Z

Avoid creating commas that are on the LHS of an assignment or below an ADDR node.

These primarily were created by morph which has an IND(COMMA(x, y)) -> COMMA(x, IND(y)) transformation that did not pay attention to whether it was on the left of an assignment.
The IR shape is pretty odd; the RHS of these commas are not actually evaluated in the traditional sense, but happen as part of the parent ASG, so the semantics of the construct ends up being confusing.
Additionally it complicates #83590.

I initially tried removing the IND(COMMA(x, y)) -> COMMA(x, IND(y)) transformation entirely but this has a lot of diffs -- probably a bit of work to teach downstream phases about these new commas would be required. I gave up after doing that in a few separate places.

A bit too many downstream things to teach about this to avoid diffs.

ghost · 2023-03-23T12:54:05Z

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

Issue Details

Avoid creating commas that are on the LHS of an assignment or below an ADDR node.

These primarily were created by morph which has an IND(COMMA(x, y)) -> COMMA(x, IND(y)) transformation that did not pay attention to whether it was on the left of an assignment.
The IR shape is pretty odd; the RHS of these commas are not actually evaluated in the traditional sense, but happen as part of the parent ASG, so the semantics of the construct ends up being confusing.
Additionally it complicates #83590.

I initially tried removing the IND(COMMA(x, y)) -> COMMA(x, IND(y)) transformation entirely but this has a lot of diffs -- probably a bit of work to teach downstream phases about these new commas would be required. I gave up after doing that in a few separate places.

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

jakobbotsch · 2023-03-23T12:55:16Z

src/coreclr/jit/morph.cpp

-        if ((op1 != nullptr) && fgIsCommaThrow(op1, true))
+        bool mightBeLocation = (tree->OperIsIndir() || tree->OperIsLocal()) && ((tree->gtFlags & GTF_DONT_CSE) != 0);
+        if (!mightBeLocation)
        {
-            GenTree* propagatedThrow = fgPropagateCommaThrow(tree, op1->AsOp(), GTF_EMPTY);
-            if (propagatedThrow != nullptr)
+            if ((op1 != nullptr) && fgIsCommaThrow(op1, true))
            {
-                return propagatedThrow;
+                GenTree* propagatedThrow = fgPropagateCommaThrow(tree, op1->AsOp(), GTF_EMPTY);
+                if (propagatedThrow != nullptr)
+                {
+                    return propagatedThrow;
+                }
            }
-        }

-        if ((op2 != nullptr) && fgIsCommaThrow(op2, true))
-        {
-            GenTree* propagatedThrow = fgPropagateCommaThrow(tree, op2->AsOp(), op1->gtFlags & GTF_ALL_EFFECT);
-            if (propagatedThrow != nullptr)
+            if ((op2 != nullptr) && fgIsCommaThrow(op2, true))
            {
-                return propagatedThrow;
+                GenTree* propagatedThrow = fgPropagateCommaThrow(tree, op2->AsOp(), op1->gtFlags & GTF_ALL_EFFECT);
+                if (propagatedThrow != nullptr)
+                {
+                    return propagatedThrow;
+                }


This transform was creating illegal IR like the following:

fgMorphTree BB01, STMT00017 (after) [000098] -ACXG+----- ▌ ASG long [000134] --CXG+-N--- ├──▌ COMMA long [000137] --CXG+----- │ ├──▌ CALL help void CORINFO_HELP_OVERFLOW [000136] -----+----- arg0 in rcx │ │ └──▌ CNS_INT int 0 [000142] -----+----- │ └──▌ CNS_INT long 0 [000095] -----+----- └──▌ CNS_INT long 0xFFFFFFFF

jakobbotsch · 2023-03-23T14:48:14Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

azure-pipelines · 2023-03-23T14:48:45Z

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch · 2023-03-23T20:13:22Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

azure-pipelines · 2023-03-23T20:13:41Z

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch · 2023-03-23T23:12:54Z

This passed jitstress with a failure that looked like #82690.

cc @dotnet/jit-contrib PTAL @AndyAyersMS @SingleAccretion

Use of GTF_DONT_CSE to detect locations is not that elegant but it is on par with other places in morph...

jakobbotsch · 2023-03-24T09:35:29Z

Diffs seem to be due to new/missed CSE opportunities

The TP regressions are the same as in #83590 (comment). Apparently removing the call to MorphBlock from PrepareDst is enough to cause MSVC to change its inlining decisions for fgMorphCopyBlock and fgMorphInitBlock.

AndyAyersMS

These can still be created explicitly?

Wondering at what point we can transition from "avoid creating more of these" to "avoid having these all together" ?

jakobbotsch · 2023-03-24T15:29:41Z

These can still be created explicitly?

Wondering at what point we can transition from "avoid creating more of these" to "avoid having these all together" ?

There are no known cases where we create these anymore, and we have asserts in block morphing (and gtSplitTree) that the case doesn't happen anymore. So effectively after this PR it is an "illegal" IR shape and we can/do rely on it to not be created.

SingleAccretion

There is also some code which can be deleted/simplified:

runtime/src/coreclr/jit/valuenum.cpp

Lines 9842 to 9844 in 768c3ff

    
           // We have to handle the case where the LHS is a comma.  In that case, we don't evaluate the comma, 
        
           // and we're really just interested in the effective value. 
        
           lhs = lhs->gtEffectiveVal();

runtime/src/coreclr/jit/optimizer.cpp

Line 8620 in 768c3ff

GenTree* lhs = tree->AsOp()->gtOp1->gtEffectiveVal(/*commaOnly*/ true);

Nice to see the IR getting better :).

SingleAccretion · 2023-03-24T16:05:33Z

src/coreclr/jit/morph.cpp

@@ -11504,6 +11502,13 @@ GenTree* Compiler::fgPropagateCommaThrow(GenTree* parent, GenTreeOp* commaThrow,
    assert(fgGlobalMorph);
    assert(fgIsCommaThrow(commaThrow));

+    bool mightBeLocation = (parent->OperIsLocal() || parent->OperIsIndir()) && ((parent->gtFlags & GTF_DONT_CSE) != 0);


parent->OperIsLocal looks a bit unnecessary as LCL_VAR/LCL_FLD LHSs cannot have children, and STORE_LCL_VAR/FLD (when those start appearing here) won't need to be restricted like this.

Removed that check.

SingleAccretion · 2023-03-24T16:07:18Z

src/coreclr/jit/morphblock.cpp

+        // Data is a location.
+        tree->Data()->gtFlags |= GTF_DONT_CSE;


If we're relying on this NO_CSE, seems prudent to set it in import as well.

I decided to leave this here only, I think we should either set it on all locations in the importer or none of them, and today we don't set it on any of them. As you probably know we rely on this flag for the similar purpose in fgMorphLocalVar as well.

I guess today only the IND form of locations is allowed for STORE_DYN_BLK, but you could imagine if we allowed locations in general then something like STORE_DYN_BLK(LCL_VAR int, LCL_VAR int, 4) would have also been incorrectly handled without this.

jakobbotsch · 2023-03-25T11:05:34Z

Failures are known according to build analysis.

jakobbotsch added 3 commits March 23, 2023 13:32

Remove IND(COMMA(x, y)) -> COMMA(x, IND(y)) transform entirely

6af4141

Revert the "entirely" removal.

89910ac

A bit too many downstream things to teach about this to avoid diffs.

Just disable the transform for locations

5f19c59

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 23, 2023

ghost assigned jakobbotsch Mar 23, 2023

jakobbotsch commented Mar 23, 2023

View reviewed changes

jakobbotsch changed the title ~~JIT: Disallow "location" commas~~ JIT: Stop creating "location" commas Mar 23, 2023

build-analysis bot mentioned this pull request Mar 23, 2023

[release/6.0] Doublelinklist GC failures on Mono #83245

Closed

Mark STORE_DYN_BLK locations as GTF_DONT_CSE

ce62726

Revert post-phase checks

298c2d1

jakobbotsch marked this pull request as ready for review March 23, 2023 23:12

Mitigate some TP

768c3ff

jakobbotsch mentioned this pull request Mar 24, 2023

JIT: Generalize handling of commas in block morphing #83590

Merged

AndyAyersMS approved these changes Mar 24, 2023

View reviewed changes

SingleAccretion reviewed Mar 24, 2023

View reviewed changes

Address feedback

2ec8474

SingleAccretion approved these changes Mar 24, 2023

View reviewed changes

build-analysis bot mentioned this pull request Mar 24, 2023

WasmTestOnBrowser-System.* test failures in CI #83655

Closed

jakobbotsch merged commit d795694 into dotnet:main Mar 25, 2023

jakobbotsch deleted the disallow-location-commas branch March 25, 2023 11:06

EgorBo mentioned this pull request Mar 28, 2023

Regressions in System.Collections.CreateAddAndClear<Int32> #84029

Closed

EgorBo mentioned this pull request Mar 28, 2023

[Perf] Linux/x64: 1 Improvement on 3/25/2023 2:21:39 PM dotnet/perf-autofiling-issues#14501

Closed

SingleAccretion mentioned this pull request Apr 20, 2023

Add a test for STORE_DYN_BLK(..., IND(null), ...) #85103

Merged

AndyAyersMS mentioned this pull request Apr 20, 2023

Investigate microbenchmarks that regress with PGO enabled #84264

Closed

ghost locked as resolved and limited conversation to collaborators Apr 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Stop creating "location" commas #83814

JIT: Stop creating "location" commas #83814

jakobbotsch commented Mar 23, 2023

ghost commented Mar 23, 2023

jakobbotsch Mar 23, 2023

jakobbotsch commented Mar 23, 2023

azure-pipelines bot commented Mar 23, 2023

jakobbotsch commented Mar 23, 2023

azure-pipelines bot commented Mar 23, 2023

jakobbotsch commented Mar 23, 2023

jakobbotsch commented Mar 24, 2023

AndyAyersMS left a comment

jakobbotsch commented Mar 24, 2023

SingleAccretion left a comment

SingleAccretion Mar 24, 2023

jakobbotsch Mar 24, 2023

SingleAccretion Mar 24, 2023

jakobbotsch Mar 24, 2023

jakobbotsch commented Mar 25, 2023

	// We have to handle the case where the LHS is a comma. In that case, we don't evaluate the comma,
	// and we're really just interested in the effective value.
	lhs = lhs->gtEffectiveVal();

		// Data is a location.
		tree->Data()->gtFlags \|= GTF_DONT_CSE;

JIT: Stop creating "location" commas #83814

JIT: Stop creating "location" commas #83814

Conversation

jakobbotsch commented Mar 23, 2023

ghost commented Mar 23, 2023

jakobbotsch Mar 23, 2023

Choose a reason for hiding this comment

jakobbotsch commented Mar 23, 2023

azure-pipelines bot commented Mar 23, 2023

jakobbotsch commented Mar 23, 2023

azure-pipelines bot commented Mar 23, 2023

jakobbotsch commented Mar 23, 2023

jakobbotsch commented Mar 24, 2023

AndyAyersMS left a comment

Choose a reason for hiding this comment

jakobbotsch commented Mar 24, 2023

SingleAccretion left a comment

Choose a reason for hiding this comment

SingleAccretion Mar 24, 2023

Choose a reason for hiding this comment

jakobbotsch Mar 24, 2023

Choose a reason for hiding this comment

SingleAccretion Mar 24, 2023

Choose a reason for hiding this comment

jakobbotsch Mar 24, 2023

Choose a reason for hiding this comment

jakobbotsch commented Mar 25, 2023