JIT: Stop sinking stores below commas in impStoreStruct

[jakobbotsch]

The following logic in the importer creates unnaturally/incorrectly typed COMMA nodes:

runtime/src/coreclr/jit/importer.cpp

Lines 962 to 993 in 790c4c0

	else if (src->OperIs(GT_COMMA))
	{
	if (pAfterStmt)
	{
	// Insert op1 after '*pAfterStmt'
	Statement* newStmt = gtNewStmt(src->AsOp()->gtOp1, usedDI);
	fgInsertStmtAfter(block, *pAfterStmt, newStmt);
	*pAfterStmt = newStmt;
	}
	else if (impLastStmt != nullptr)
	{
	// Do the side-effect as a separate statement.
	impAppendTree(src->AsOp()->gtOp1, curLevel, usedDI);
	}
	else
	{
	// In this case we have neither been given a statement to insert after, nor are we
	// in the importer where we can append the side effect.
	// Instead, we're going to sink the store below the COMMA.
	store->Data() = src->AsOp()->gtOp2;
	src->AsOp()->gtOp2 = impStoreStruct(store, curLevel, pAfterStmt, usedDI, block);
	src->SetAllEffectsFlags(src->AsOp()->gtOp1, src->AsOp()->gtOp2);
	gtUpdateNodeSideEffects(store);
	return src;
	}
	// Evaluate the second thing using recursion.
	store->Data() = src->AsOp()->gtOp2;
	gtUpdateNodeSideEffects(store);
	return impStoreStruct(store, curLevel, pAfterStmt, usedDI, block);
	}

It changes for instance

               [000006] --CXG------                         ▌  COMMA     simd12
               [000005] H-CXG------                         ├──▌  CALL help int    CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000003] ----------- arg0                    │  ├──▌  CNS_INT   int    0x8A2B390
               [000004] ----------- arg1                    │  └──▌  CNS_INT   int    1
               [000001] I---G------                         └──▌  IND       simd12
               [000000] H----------                            └──▌  CNS_INT(h) int    0x8601620 static Fseq[s]

to

               [000006] -ACXG------                         ▌  COMMA     simd12
               [000005] H-CXG------                         ├──▌  CALL help int    CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000003] ----------- arg0                    │  ├──▌  CNS_INT   int    0x8A2B390
               [000004] ----------- arg1                    │  └──▌  CNS_INT   int    1
               [000087] DA--G------                         └──▌  STORE_LCL_VAR simd12<System.Numerics.Vector3> V02 tmp1         
               [000001] I---G------                            └──▌  IND       simd12
               [000000] H----------                               └──▌  CNS_INT(h) int    0x8601620 static Fseq[s]

We would usually expect [000006] to be TYP_VOID in the latter tree, as evidenced by gtExtractSideEffList. Morph also has code to compensate:

runtime/src/coreclr/jit/morph.cpp

Lines 9665 to 9669 in 790c4c0

	/* Special case: trees that don't produce a value */
	if (op2->OperIsStore() || (op2->OperGet() == GT_COMMA && op2->TypeGet() == TYP_VOID) || fgIsThrow(op2))
	{
	typ = tree->gtType = TYP_VOID;
	}

We should fix this; we can likely just avoid sinking these stores below the COMMAs -- this used to be necessary because block morphing did not handle the pattern, but it should be handled after #83590.

#91443 has an example bug because of this JIT IR oddity.

[ghost]

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

Issue Details

---

The following logic in the importer creates unnaturally/incorrectly typed COMMA nodes:

runtime/src/coreclr/jit/importer.cpp

Lines 962 to 993 in 790c4c0

	else if (src->OperIs(GT_COMMA))
	{
	if (pAfterStmt)
	{
	// Insert op1 after '*pAfterStmt'
	Statement* newStmt = gtNewStmt(src->AsOp()->gtOp1, usedDI);
	fgInsertStmtAfter(block, *pAfterStmt, newStmt);
	*pAfterStmt = newStmt;
	}
	else if (impLastStmt != nullptr)
	{
	// Do the side-effect as a separate statement.
	impAppendTree(src->AsOp()->gtOp1, curLevel, usedDI);
	}
	else
	{
	// In this case we have neither been given a statement to insert after, nor are we
	// in the importer where we can append the side effect.
	// Instead, we're going to sink the store below the COMMA.
	store->Data() = src->AsOp()->gtOp2;
	src->AsOp()->gtOp2 = impStoreStruct(store, curLevel, pAfterStmt, usedDI, block);
	src->SetAllEffectsFlags(src->AsOp()->gtOp1, src->AsOp()->gtOp2);
	gtUpdateNodeSideEffects(store);
	return src;
	}
	// Evaluate the second thing using recursion.
	store->Data() = src->AsOp()->gtOp2;
	gtUpdateNodeSideEffects(store);
	return impStoreStruct(store, curLevel, pAfterStmt, usedDI, block);
	}

It changes for instance

               [000006] --CXG------                         ▌  COMMA     simd12
               [000005] H-CXG------                         ├──▌  CALL help int    CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000003] ----------- arg0                    │  ├──▌  CNS_INT   int    0x8A2B390
               [000004] ----------- arg1                    │  └──▌  CNS_INT   int    1
               [000001] I---G------                         └──▌  IND       simd12
               [000000] H----------                            └──▌  CNS_INT(h) int    0x8601620 static Fseq[s]

to

               [000006] -ACXG------                         ▌  COMMA     simd12
               [000005] H-CXG------                         ├──▌  CALL help int    CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000003] ----------- arg0                    │  ├──▌  CNS_INT   int    0x8A2B390
               [000004] ----------- arg1                    │  └──▌  CNS_INT   int    1
               [000087] DA--G------                         └──▌  STORE_LCL_VAR simd12<System.Numerics.Vector3> V02 tmp1         
               [000001] I---G------                            └──▌  IND       simd12
               [000000] H----------                               └──▌  CNS_INT(h) int    0x8601620 static Fseq[s]

We would usually expect [000006] to be TYP_VOID in the latter tree, as evidenced by gtExtractSideEffList. Morph also has code to compensate:

runtime/src/coreclr/jit/morph.cpp

Lines 9665 to 9669 in 790c4c0

	/* Special case: trees that don't produce a value */
	if (op2->OperIsStore() || (op2->OperGet() == GT_COMMA && op2->TypeGet() == TYP_VOID) || fgIsThrow(op2))
	{
	typ = tree->gtType = TYP_VOID;
	}

We should fix this; we can likely just avoid sinking these stores below the COMMAs anymore -- this used to be necessary because block morphing did not handle the pattern, but it should be handled after #83590.

Author:	jakobbotsch
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[jakobbotsch]

#106380 is another example of a bug due to this issue.

We should fix this; we can likely just avoid sinking these stores below the COMMAs -- this used to be necessary because block morphing did not handle the pattern, but it should be handled after #83590.

Another thing is that, since LIR does not support TYP_STRUCT edges beyond some very restricted patterns, we should ensure that the handling for these struct operations properly handle interfering side effects.