Fix for a jit liveness bug. #24282

erozenfeld · 2019-04-26T23:19:24Z

fgRemoveDeadStore has special logic for removing dead assignments
whose rhs was of type TYP_STRUCT:

Lines 2264 to 2274 in 311b5e2

    
           if (rhsNode->TypeGet() == TYP_STRUCT) 
        
           { 
        
               // This is a block assignment. An indirection of the rhs is not considered to 
        
               // happen until the assignment, so we will extract the side effects from only 
        
               // the address. 
        
               if (rhsNode->OperIsIndir()) 
        
               { 
        
                   assert(rhsNode->OperGet() != GT_NULLCHECK); 
        
                   rhsNode = rhsNode->AsIndir()->Addr(); 
        
               } 
        
           }

That logic was applied to "normal"
assignments (i.e., direct children of GT_STMT) but not to "internal"
assignments (e.g., children of GT_COMMA).
The test case has an internal assignment and, because this logic wasn't
applied, we ended up with a standalone GT_IND of type TYP_STRUCT
that the register allocator can't handle.

This change apples the missing logic to "internal" assignments.

The second commit cleans up fgRemoveDeadStore.

Fixes #24253.

`fgRemoveDeadStore` has special logic for removing dead assignments whose rhs was of type `TYP_STRUCT`: https://github.com/dotnet/coreclr/blob/311b5e2fe413c6c74a2a3680ab54d8a978651472/src/jit/liveness.cpp#L2264-L2274 That logic was applied to "normal" assignments (i.e., direct children of `GT_STMT`) but not to "internal" assignments (e.g., children of `GT_COMMA`). The test case has an internal assignment and, because this logic wasn't applied, we ended up with a standalone `GT_IND` of type `TYP_STRUCT` that the register allocator can't handle. This change apples the missing logic to "internal" assignments. Fixes #24253.

erozenfeld · 2019-04-26T23:20:13Z

No x64 pmi diffs in frameworks and benchmarks.
I will try to check SPMI diffs as well.

erozenfeld · 2019-04-26T23:21:43Z

@dotnet/jit-contrib @sandreenko @CarolEidt PTAL
It's probably easier to review the two commits individually: the first one is the actual fix and the second one is the cleanup of confusing surrounding code.

CarolEidt

LGTM - much cleaner!

CarolEidt · 2019-04-26T23:59:18Z

src/jit/liveness.cpp

+        GenTree* sideEffList = nullptr;
+        if (rhsNode->gtFlags & GTF_SIDE_EFFECT)
+        {
+// Extract the side effects


I would suggest moving this comment below the DEBUG code - or you could add a CLANG_FORMAT_COMMENT_ANCHOR;

CarolEidt · 2019-04-27T00:00:18Z

src/jit/liveness.cpp


-                    /* Replace the assignment statement with the list of side effects */
-                    noway_assert(sideEffList->gtOper != GT_STMT);
+                /* Replace the assignment statement with the list of side effects */


While you're here you could make these C++ style comments.

CarolEidt · 2019-04-27T00:02:27Z

src/jit/liveness.cpp

            }
            else
            {
-            NO_SIDE_EFFECTS:


I love getting rid of gotos!!

sandreenko · 2019-04-27T00:04:41Z

src/jit/liveness.cpp

+                    // the address.
+                    if (rhsNode->OperIsIndir())
+                    {
+                        assert(rhsNode->OperGet() != GT_NULLCHECK);


Why should not it be a nullcheck?

We never create GT_NULLCHECK as a rhs of an assignment. I guess this assert just makes sure we don't accidentally remove a null check.

sandreenko · 2019-04-27T00:06:53Z

src/jit/liveness.cpp

-            /* Check for side effects */
-
-            if (rhsNode->gtFlags & GTF_SIDE_EFFECT)
+            if (sideEffList)


if (sideEffList != nullptr)

sandreenko · 2019-04-27T00:08:34Z

src/jit/liveness.cpp

+                noway_assert(sideEffList->gtOper != GT_STMT);

-                    *pTree = compCurStmt->gtStmtExpr = sideEffList;
+                *pTree = compCurStmt->gtStmtExpr = sideEffList;


Is it possible that pTree != &compCurStmt->gtStmtExpr?

pTree is an address of a local GenTree* variable in the caller, (e.g., address of tree in fgComputeLife.

sandreenko

The fix looks good, thanks for the cleaning.

Remove duplication and a couple of goto's from `fgRemoveDeadStore`. This part was very confusing: ` /* If this is GT_CATCH_ARG saved to a local var don't bother */ JITDUMP("removing stmt with no side effects\n"); if (asgNode->gtFlags & GTF_ORDER_SIDEEFF) { if (rhsNode->gtOper == GT_CATCH_ARG) { goto EXTRACT_SIDE_EFFECTS; } } ` The `goto` was to the preceding `if` and was useless since the call to `gtExtractSideEffList(rhsNode, &sideEffList);` couldn't possibly find side effects because we already checked that `rhsNode->gtFlags & GTF_SIDE_EFFECT == 0`.

erozenfeld · 2019-04-27T02:25:26Z

No desktop SPMI diffs.

erozenfeld · 2019-04-27T21:26:08Z

CoreFX test failed with
System.Net.Security.Tests.SslStreamCredentialCacheTest.SslStream_SameCertUsedForClientAndServer_Ok [FAIL]
System.TimeoutException : Task timed out after 15000ms

erozenfeld · 2019-04-27T21:26:27Z

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests

erozenfeld · 2019-04-27T23:12:01Z

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests

erozenfeld · 2019-04-28T03:48:14Z

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests

briansull

Looks Good

sandreenko · 2019-04-29T16:01:49Z

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests (the broken machine is disabled now).

Fix for a jit liveness bug. Commit migrated from dotnet/coreclr@78f53be

CarolEidt approved these changes Apr 27, 2019

View reviewed changes

sandreenko reviewed Apr 27, 2019

View reviewed changes

sandreenko approved these changes Apr 27, 2019

View reviewed changes

erozenfeld force-pushed the Fix24253 branch from 67d87fc to dc389a8 Compare April 27, 2019 00:36

briansull approved these changes Apr 29, 2019

View reviewed changes

erozenfeld merged commit 78f53be into dotnet:master Apr 29, 2019

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#24282 from erozenfeld/Fix24253

650c3cb

Fix for a jit liveness bug. Commit migrated from dotnet/coreclr@78f53be

	if (rhsNode->TypeGet() == TYP_STRUCT)
	{
	// This is a block assignment. An indirection of the rhs is not considered to
	// happen until the assignment, so we will extract the side effects from only
	// the address.
	if (rhsNode->OperIsIndir())
	{
	assert(rhsNode->OperGet() != GT_NULLCHECK);
	rhsNode = rhsNode->AsIndir()->Addr();
	}
	}

Fix for a jit liveness bug. #24282

Fix for a jit liveness bug. #24282

Uh oh!

Conversation

erozenfeld commented Apr 26, 2019

Uh oh!

erozenfeld commented Apr 26, 2019

Uh oh!

erozenfeld commented Apr 26, 2019

Uh oh!

CarolEidt 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!

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!

sandreenko left a comment

Choose a reason for hiding this comment

Uh oh!

erozenfeld commented Apr 27, 2019

Uh oh!

erozenfeld commented Apr 27, 2019

Uh oh!

erozenfeld commented Apr 27, 2019

Uh oh!

erozenfeld commented Apr 27, 2019

Uh oh!

erozenfeld commented Apr 28, 2019

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

sandreenko commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants