Struct & SIMD improvements (dotnet/coreclr#22255)

* [WIP] Struct & SIMD improvements

- Enable CSE of struct values when handle is available (and add code to get the handle of HW SIMD types)
- Don't require block nodes for SIMD assignments
- Don't set `GTF_GLOB_REF` on `GT_OBJ` if it is local
- Set `lvRegStruct` on promoted SIMD fields
- Add tests for dotnet/coreclr#19910 (fixed with this PR) and dotnet/coreclr#3539 & dotnet/coreclr#19438 (fixed with dotnet/coreclr#21314)
- Additional cleanup

Fix dotnet/coreclr#19910

Commit migrated from dotnet/coreclr@3d4a1d5

src/coreclr/src/jit/codegencommon.cpp

@@ -4534,15 +4534,7 @@ void CodeGen::genCheckUseBlockInit()
  unless they are untracked GC type or structs that contain GC pointers */
  CLANG_FORMAT_COMMENT_ANCHOR;
 
-#if FEATURE_SIMD
- // TODO-1stClassStructs
- // This is here to duplicate previous behavior, where TYP_SIMD8 locals
- // were not being re-typed correctly.
- if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT) || (varDsc->lvType == TYP_SIMD8)) &&
-#else // !FEATURE_SIMD
- if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT)) &&
-#endif // !FEATURE_SIMD
- varDsc->lvOnFrame &&
+ if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT)) && varDsc->lvOnFrame &&
  (!varDsc->lvIsTemp || varTypeIsGC(varDsc->TypeGet()) || (varDsc->lvStructGcCount > 0)))
  {
  varDsc->lvMustInit = true;

src/coreclr/src/jit/gentree.cpp

@@ -16529,15 +16529,45 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree)
  if (varTypeIsSIMD(tree))
  {
  structHnd = gtGetStructHandleForSIMD(tree->gtType, TYP_FLOAT);
+#ifdef FEATURE_HW_INTRINSICS
+ if (structHnd == NO_CLASS_HANDLE)
+ {
+ structHnd = gtGetStructHandleForHWSIMD(tree->gtType, TYP_FLOAT);
+ }
+#endif
  }
  else
 #endif
  {
+ // Attempt to find a handle for this expression.
+ // We can do this for an array element indirection, or for a field indirection.
  ArrayInfo arrInfo;
  if (TryGetArrayInfo(tree->AsIndir(), &arrInfo))
  {
  structHnd = EncodeElemType(arrInfo.m_elemType, arrInfo.m_elemStructType);
  }
+ else
+ {
+ GenTree* addr = tree->AsIndir()->Addr();
+ if ((addr->OperGet() == GT_ADD) && addr->gtGetOp2()->OperIs(GT_CNS_INT))
+ {
+ FieldSeqNode* fieldSeq = addr->gtGetOp2()->AsIntCon()->gtFieldSeq;
+
+ if (fieldSeq != nullptr)
+ {
+ while (fieldSeq->m_next != nullptr)
+ {
+ fieldSeq = fieldSeq->m_next;
+ }
+ if (fieldSeq != FieldSeqStore::NotAField() && !fieldSeq->IsPseudoField())
+ {
+ CORINFO_FIELD_HANDLE fieldHnd = fieldSeq->m_fieldHnd;
+ CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &structHnd);
+ assert(fieldCorType == CORINFO_TYPE_VALUECLASS);
+ }
+ }
+ }
+ }
  }
  break;
 #ifdef FEATURE_SIMD

src/coreclr/src/jit/gentree.h

@@ -4712,22 +4712,28 @@ struct GenTreeObj : public GenTreeBlk
  }
  }
 
+ void Init()
+ {
+ // By default, an OBJ is assumed to be a global reference, unless it is local.
+ GenTreeLclVarCommon* lcl = Addr()->IsLocalAddrExpr();
+ if ((lcl == nullptr) || ((lcl->gtFlags & GTF_GLOB_EFFECT) != 0))
+ {
+ gtFlags |= GTF_GLOB_REF;
+ }
+ noway_assert(gtClass != NO_CLASS_HANDLE);
+ _gtGcPtrCount = UINT32_MAX;
+ }
+
  GenTreeObj(var_types type, GenTree* addr, CORINFO_CLASS_HANDLE cls, unsigned size)
  : GenTreeBlk(GT_OBJ, type, addr, size), gtClass(cls)
  {
- // By default, an OBJ is assumed to be a global reference.
- gtFlags |= GTF_GLOB_REF;
- noway_assert(cls != NO_CLASS_HANDLE);
- _gtGcPtrCount = UINT32_MAX;
+ Init();
  }
 
  GenTreeObj(var_types type, GenTree* addr, GenTree* data, CORINFO_CLASS_HANDLE cls, unsigned size)
  : GenTreeBlk(GT_STORE_OBJ, type, addr, data, size), gtClass(cls)
  {
- // By default, an OBJ is assumed to be a global reference.
- gtFlags |= GTF_GLOB_REF;
- noway_assert(cls != NO_CLASS_HANDLE);
- _gtGcPtrCount = UINT32_MAX;
+ Init();
  }
 
 #if DEBUGGABLE_GENTREE

src/coreclr/src/jit/importer.cpp

@@ -618,21 +618,8 @@ inline void Compiler::impAppendStmt(GenTree* stmt, unsigned chkLevel)
  // Since we don't know what it assigns to, we need to spill global refs.
  spillGlobEffects = true;
  }
- else if (!expr->OperIsBlkOp())
+ else if ((lhs->gtFlags & GTF_GLOB_REF) != 0)
  {
- // If we are assigning to a global ref, we have to spill global refs on stack
- if ((lhs->gtFlags & GTF_GLOB_REF) != 0)
- {
- spillGlobEffects = true;
- }
- }
- else if ((lhs->OperIsBlk() && !lhs->AsBlk()->HasGCPtr()) ||
- ((lhs->OperGet() == GT_LCL_VAR) &&
- (lvaTable[lhs->AsLclVarCommon()->gtLclNum].lvStructGcCount == 0)))
- {
- // TODO-1stClassStructs: Previously, spillGlobEffects was set to true for
- // GT_INITBLK and GT_COPYBLK, but this is overly conservative, and should be
- // revisited. (Note that it was NOT set to true for GT_COPYOBJ.)
  spillGlobEffects = true;
  }
  }
@@ -1389,8 +1376,6 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
  }
  if (dest == nullptr)
  {
- // TODO-1stClassStructs: We shouldn't really need a block node as the destination
- // if this is a known struct type.
  if (asgType == TYP_STRUCT)
  {
  dest = gtNewObjNode(structHnd, destAddr);
@@ -1401,10 +1386,6 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
  dest->gtFlags &= ~GTF_GLOB_REF;
  dest->gtFlags |= (destAddr->gtFlags & GTF_GLOB_REF);
  }
- else if (varTypeIsStruct(asgType))
- {
- dest = new (this, GT_BLK) GenTreeBlk(GT_BLK, asgType, destAddr, genTypeSize(asgType));
- }
  else
  {
  dest = gtNewOperNode(GT_IND, asgType, destAddr);
@@ -14527,9 +14508,8 @@ void Compiler::impImportBlockCode(BasicBlock* block)
  assert(!"Unexpected fieldAccessor");
  }
 
- // Create the member assignment, unless we have a struct.
- // TODO-1stClassStructs: This could be limited to TYP_STRUCT, to avoid extra copies.
- bool deferStructAssign = varTypeIsStruct(lclTyp);
+ // Create the member assignment, unless we have a TYP_STRUCT.
+ bool deferStructAssign = (lclTyp == TYP_STRUCT);
 
  if (!deferStructAssign)
  {
@@ -16220,10 +16200,6 @@ GenTree* Compiler::impAssignSmallStructTypeToVar(GenTree* op, CORINFO_CLASS_HAND
  unsigned tmpNum = lvaGrabTemp(true DEBUGARG("Return value temp for small struct return."));
  impAssignTempGen(tmpNum, op, hClass, (unsigned)CHECK_SPILL_ALL);
  GenTree* ret = gtNewLclvNode(tmpNum, lvaTable[tmpNum].lvType);
-
- // TODO-1stClassStructs: Handle constant propagation and CSE-ing of small struct returns.
- ret->gtFlags |= GTF_DONT_CSE;
-
  return ret;
 }
 

src/coreclr/src/jit/lclvars.cpp

@@ -2175,6 +2175,9 @@ void Compiler::StructPromotionHelper::PromoteStructVar(unsigned lclNum)
  // Set size to zero so that lvaSetStruct will appropriately set the SIMD-relevant fields.
  fieldVarDsc->lvExactSize = 0;
  compiler->lvaSetStruct(varNum, pFieldInfo->fldTypeHnd, false, true);
+ // We will not recursively promote this, so mark it as 'lvRegStruct' (note that we wouldn't
+ // be promoting this if we didn't think it could be enregistered.
+ fieldVarDsc->lvRegStruct = true;
  }
 #endif // FEATURE_SIMD
 
@@ -7047,8 +7050,6 @@ void Compiler::lvaDumpEntry(unsigned lclNum, FrameLayoutState curState, size_t r
  printf(" V%02u.%s(offs=0x%02x)", varDsc->lvParentLcl, eeGetFieldName(fldHnd), varDsc->lvFldOffset);
 
  lvaPromotionType promotionType = lvaGetPromotionType(parentvarDsc);
- // We should never have lvIsStructField set if it is a reg-sized non-field-addressed struct.
- assert(!varDsc->lvRegStruct);
  switch (promotionType)
  {
  case PROMOTION_TYPE_NONE:

src/coreclr/src/jit/liveness.cpp

@@ -2160,14 +2160,14 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree,
  addrNode = tree;
  }
 
- // Next, find the assignment.
+ // Next, find the assignment (i.e. if we didn't have a LocalStore)
  if (asgNode == nullptr)
  {
  if (addrNode == nullptr)
  {
  asgNode = nextNode;
  }
- else if (asgNode == nullptr)
+ else
  {
  // This may be followed by GT_IND/assign or GT_STOREIND.
  if (nextNode == nullptr)
@@ -2180,19 +2180,22 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree,
  assert(nextNode->OperGet() != GT_NULLCHECK);
  if (nextNode->OperIsStore())
  {
+ // This is a store, which takes a location and a value to be stored.
+ // It's 'rhsNode' is the value to be stored.
  asgNode = nextNode;
  if (asgNode->OperIsBlk())
  {
  rhsNode = asgNode->AsBlk()->Data();
  }
- // TODO-1stClassStructs: There should be an else clause here to handle
- // the non-block forms of store ops (GT_STORE_LCL_VAR, etc.) for which
- // rhsNode is op1. (This isn't really a 1stClassStructs item, but the
- // above was added to catch what used to be dead block ops, and that
- // made this omission apparent.)
+ else
+ {
+  // This is a non-block store.
+  rhsNode = asgNode->gtGetOp2();
+ }
  }
  else
  {
+ // This is a non-store indirection, and the assignment will com after it.
  asgNode = nextNode->gtNext;
  }
  }

src/coreclr/src/jit/lower.cpp

@@ -1765,11 +1765,7 @@ void Lowering::CheckVSQuirkStackPaddingNeeded(GenTreeCall* call)
  if (op1->OperGet() == GT_LCL_VAR_ADDR)
  {
  unsigned lclNum = op1->AsLclVarCommon()->GetLclNum();
- // TODO-1stClassStructs: This is here to duplicate previous behavior,
- // but is not needed because the scenario being quirked did not involve
- // a SIMD or enregisterable struct.
- // if(comp->lvaTable[lclNum].TypeGet() == TYP_STRUCT)
- if (varTypeIsStruct(comp->lvaTable[lclNum].TypeGet()))
+ if (comp->lvaGetDesc(lclNum)->TypeGet() == TYP_STRUCT)
  {
  // First arg is addr of a struct local.
  paddingNeeded = true;