Fix VN incorrect optimizations with a new JitEEInterface function. (#…

…57282)

* Add/return tests.

* improve the test naming.

* Add a new JitEE method.

with a naive implementation so far.

* Handle generically dissimilar type concerns as well as handle derived type case in the managed compiler

* Fix the field and Unsafe issues.

* fix some failures.

It appeared that we have many trees where we generate unique VN for rhs and don't inform `fgValueNumberBlockAssignment` about it.
For example, for
```
N005 ( 10,  8) [000033] -A--G---R---              *  ASG       struct (copy)
N004 (  3,  2) [000031] D------N----              +--*  LCL_VAR   struct<eightByteStruct, 8> V01 loc1         d:2
N003 (  6,  5) [000030] n---G-------              \--*  IND       struct
N002 (  3,  3) [000043] ------------                 \--*  ADDR      byref
N001 (  3,  2) [000044] -------N----                    \--*  LCL_VAR   struct<largeStruct, 32> V00 loc0         u:6 (last use)
```
we will generate unique rhs but then `fgValueNumberBlockAssignment` will still try to work it as with a non-unique one.

* Fix an oooold bug in VN.

For a tree like:
```
N005 ( 11, 10) [001400] -A------R----             *  ASG       long
N004 (  6,  5) [001401] *------N-----             +--*  IND       long
N003 (  3,  3) [001402] -------------             |  \--*  ADDR      byref  Zero Fseq[_00]
N002 (  3,  2) [001403] U------N-----             |     \--*  LCL_VAR   struct<System.Runtime.Intrinsics.Vector128`1[Byte], 16> V45 tmp38        ud:3->4
N001 (  1,  1) [001404] -------------             \--*  LCL_VAR   long   V69 tmp62        u:2 (last use)
```
SSA was working correctly and printing that `001403` is using SSA-3 and defining SSA-4. However, this code, for some reason, was writing result as a define for SSA-3.

* Add new test cases and a fix for them.

* correct a test copy paste

* response review.

Co-authored-by: David Wrighton <davidwr@microsoft.com>

src/coreclr/ToolBox/superpmi/superpmi-shared/lwmlist.h

@@ -157,6 +157,7 @@ LWM(TryResolveToken, Agnostic_CORINFO_RESOLVED_TOKENin, TryResolveTokenValue)
 LWM(SatisfiesClassConstraints, DWORDLONG, DWORD)
 LWM(SatisfiesMethodConstraints, DLDL, DWORD)
 LWM(GetUnmanagedCallConv, MethodOrSigInfoValue, DD)
+LWM(DoesFieldBelongToClass, DLDL, DWORD)
 DENSELWM(SigInstHandleMap, DWORDLONG)
 
 #undef LWM

src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp

@@ -6265,6 +6265,40 @@ DWORD MethodContext::repGetExpectedTargetArchitecture()
  return value;
 }
 
+void MethodContext::recDoesFieldBelongToClass(CORINFO_FIELD_HANDLE fld, CORINFO_CLASS_HANDLE cls, bool result)
+{
+ if (DoesFieldBelongToClass == nullptr)
+ DoesFieldBelongToClass = new LightWeightMap<DLDL, DWORD>();
+
+ DLDL key;
+ ZeroMemory(&key, sizeof(key)); // Zero key including any struct padding
+ key.A = CastHandle(fld);
+ key.B = CastHandle(cls);
+
+ DWORD value = (DWORD)result;
+ DoesFieldBelongToClass->Add(key, value);
+ DEBUG_REC(dmpDoesFieldBelongToClass(key, result));
+}
+
+void MethodContext::dmpDoesFieldBelongToClass(DLDL key, bool value)
+{
+ printf("DoesFieldBelongToClass key fld=%016llX, cls=%016llx, result=%d", key.A, key.B, value);
+}
+
+bool MethodContext::repDoesFieldBelongToClass(CORINFO_FIELD_HANDLE fld, CORINFO_CLASS_HANDLE cls)
+{
+ DLDL key;
+ ZeroMemory(&key, sizeof(key)); // Zero key including any struct padding
+ key.A = CastHandle(fld);
+ key.B = CastHandle(cls);
+
+ AssertMapAndKeyExist(DoesFieldBelongToClass, key, ": key %016llX %016llX", key.A, key.B);
+
+ bool value = (bool)DoesFieldBelongToClass->Get(key);
+ DEBUG_REP(dmpDoesFieldBelongToClass(key, value));
+ return value;
+}
+
 void MethodContext::recIsValidToken(CORINFO_MODULE_HANDLE module, unsigned metaTOK, bool result)
 {
  if (IsValidToken == nullptr)

src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h

@@ -769,6 +769,10 @@ class MethodContext
  void dmpGetExpectedTargetArchitecture(DWORD key, DWORD result);
  DWORD repGetExpectedTargetArchitecture();
 
+ void recDoesFieldBelongToClass(CORINFO_FIELD_HANDLE fld, CORINFO_CLASS_HANDLE cls, bool result);
+ void dmpDoesFieldBelongToClass(DLDL key, bool value);
+ bool repDoesFieldBelongToClass(CORINFO_FIELD_HANDLE fld, CORINFO_CLASS_HANDLE cls);
+
  void recIsValidToken(CORINFO_MODULE_HANDLE module, unsigned metaTOK, bool result);
  void dmpIsValidToken(DLD key, DWORD value);
  bool repIsValidToken(CORINFO_MODULE_HANDLE module, unsigned metaTOK);
@@ -1063,7 +1067,7 @@ enum mcPackets
  Packet_TryResolveToken = 158, // Added 4/26/2016
  Packet_SatisfiesClassConstraints = 110,
  Packet_SatisfiesMethodConstraints = 111,
- Packet_ShouldEnforceCallvirtRestriction = 112, // Retired 2/18/2020
+ Packet_DoesFieldBelongToClass  = 112, // Added 8/12/2021
  Packet_SigInstHandleMap = 184,
  Packet_AllocPgoInstrumentationBySchema = 186, // Added 1/4/2021
  Packet_GetPgoInstrumentationResults = 187, // Added 1/4/2021

src/coreclr/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp

@@ -2010,3 +2010,13 @@ bool interceptor_ICJI::notifyInstructionSetUsage(CORINFO_InstructionSet instruct
 {
  return original_ICorJitInfo->notifyInstructionSetUsage(instructionSet, supported);
 }
+
+bool interceptor_ICJI::doesFieldBelongToClass(
+ CORINFO_FIELD_HANDLE fldHnd,
+ CORINFO_CLASS_HANDLE cls)
+{
+ mc->cr->AddCall("doesFieldBelongToClass");
+ bool result = original_ICorJitInfo->doesFieldBelongToClass(fldHnd, cls);
+ mc->recDoesFieldBelongToClass(fldHnd, cls, result);
+ return result;
+}

src/coreclr/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp

@@ -1389,3 +1389,11 @@ uint32_t interceptor_ICJI::getJitFlags(
  return original_ICorJitInfo->getJitFlags(flags, sizeInBytes);
 }
 
+bool interceptor_ICJI::doesFieldBelongToClass(
+ CORINFO_FIELD_HANDLE fldHnd,
+ CORINFO_CLASS_HANDLE cls)
+{
+ mcs->AddCall("doesFieldBelongToClass");
+ return original_ICorJitInfo->doesFieldBelongToClass(fldHnd, cls);
+}
+

src/coreclr/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp

@@ -1217,3 +1217,10 @@ uint32_t interceptor_ICJI::getJitFlags(
  return original_ICorJitInfo->getJitFlags(flags, sizeInBytes);
 }
 
+bool interceptor_ICJI::doesFieldBelongToClass(
+ CORINFO_FIELD_HANDLE fldHnd,
+ CORINFO_CLASS_HANDLE cls)
+{
+ return original_ICorJitInfo->doesFieldBelongToClass(fldHnd, cls);
+}
+

src/coreclr/ToolBox/superpmi/superpmi/icorjitinfo.cpp

@@ -1574,6 +1574,13 @@ uint32_t MyICJI::getJitFlags(CORJIT_FLAGS* jitFlags, uint32_t sizeInBytes)
  return ret;
 }
 
+bool MyICJI::doesFieldBelongToClass(CORINFO_FIELD_HANDLE fldHnd, CORINFO_CLASS_HANDLE cls)
+{
+ jitInstance->mc->cr->AddCall("doesFieldBelongToClass");
+ bool result = jitInstance->mc->repDoesFieldBelongToClass(fldHnd, cls);
+ return result;
+}
+
 // Runs the given function with the given parameter under an error trap
 // and returns true if the function completes successfully. We fake this
 // up a bit for SuperPMI and simply catch all exceptions.

src/coreclr/inc/corjit.h

@@ -484,6 +484,12 @@ class ICorJitInfo : public ICorDynamicInfo
  uint32_t sizeInBytes /* IN: The size of the buffer. Note that this is effectively a
  version number for the CORJIT_FLAGS value. */
  ) = 0;
+
+ // Checks if a field belongs to a given class.
+ virtual bool doesFieldBelongToClass(
+ CORINFO_FIELD_HANDLE fldHnd, /* IN: the field that we are checking */
+ CORINFO_CLASS_HANDLE cls /* IN: the class that we are checking */
+ ) = 0;
 };
 
 /**********************************************************************************/

src/coreclr/inc/icorjitinfoimpl_generated.h

@@ -710,6 +710,10 @@ uint32_t getJitFlags(
  CORJIT_FLAGS* flags,
  uint32_t sizeInBytes) override;
 
+bool doesFieldBelongToClass(
+ CORINFO_FIELD_HANDLE fldHnd,
+ CORINFO_CLASS_HANDLE cls) override;
+
 /**********************************************************************************/
 // clang-format on
 /**********************************************************************************/

src/coreclr/inc/jiteeversionguid.h

@@ -43,12 +43,12 @@ typedef const GUID *LPCGUID;
 #define GUID_DEFINED
 #endif // !GUID_DEFINED
 
-constexpr GUID JITEEVersionIdentifier = { /* c2e4a466-795d-4e64-a776-0ae7b2eed65b */
- 0xc2e4a466,
- 0x795d,
- 0x4e64,
- {0xa7, 0x76, 0x0a, 0xe7, 0xb2, 0xee, 0xd6, 0x5b}
- };
+constexpr GUID JITEEVersionIdentifier =  { /* 5ed35c58-857b-48dd-a818-7c0136dc9f73 */
+ 0x5ed35c58,
+ 0x857b,
+ 0x48dd,
+ {0xa8, 0x18, 0x7c, 0x01, 0x36, 0xdc, 0x9f, 0x73}
+};
 
 //////////////////////////////////////////////////////////////////////////////////////////////////////////
 //

src/coreclr/jit/ICorJitInfo_API_names.h

@@ -176,5 +176,6 @@ DEF_CLR_API(recordRelocation)
 DEF_CLR_API(getRelocTypeHint)
 DEF_CLR_API(getExpectedTargetArchitecture)
 DEF_CLR_API(getJitFlags)
+DEF_CLR_API(doesFieldBelongToClass)
 
 #undef DEF_CLR_API

src/coreclr/jit/ICorJitInfo_API_wrapper.hpp

@@ -1690,6 +1690,16 @@ uint32_t WrapICorJitInfo::getJitFlags(
  return temp;
 }
 
+bool WrapICorJitInfo::doesFieldBelongToClass(
+ CORINFO_FIELD_HANDLE fldHnd,
+ CORINFO_CLASS_HANDLE cls)
+{
+ API_ENTER(doesFieldBelongToClass);
+ bool temp = wrapHnd->doesFieldBelongToClass(fldHnd, cls);
+ API_LEAVE(doesFieldBelongToClass);
+ return temp;
+}
+
 /**********************************************************************************/
 // clang-format on
 /**********************************************************************************/

src/coreclr/jit/compiler.h

@@ -5208,6 +5208,8 @@ class Compiler
  // Does value-numbering for a block assignment.
  void fgValueNumberBlockAssignment(GenTree* tree);
 
+ bool fgValueNumberIsStructReinterpretation(GenTreeLclVarCommon* lhsLclVarTree, GenTreeLclVarCommon* rhsLclVarTree);
+
  // Does value-numbering for a cast tree.
  void fgValueNumberCastTree(GenTree* tree);
 

src/coreclr/jit/importer.cpp

@@ -17275,45 +17275,6 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode)
 #endif
  }
 
- // If we are inlining a method that returns a struct byref, check whether we are "reinterpreting" the
- // struct.
- GenTree* effectiveRetVal = op2->gtEffectiveVal();
- if ((returnType == TYP_BYREF) && (info.compRetType == TYP_BYREF) &&
- (effectiveRetVal->OperGet() == GT_ADDR))
- {
- GenTree* addrChild = effectiveRetVal->gtGetOp1();
- if (addrChild->OperGet() == GT_LCL_VAR)
- {
- LclVarDsc* varDsc = lvaGetDesc(addrChild->AsLclVarCommon());
-
- if (varTypeIsStruct(addrChild->TypeGet()) && !isOpaqueSIMDLclVar(varDsc))
- {
- CORINFO_CLASS_HANDLE referentClassHandle;
- CorInfoType referentType =
- info.compCompHnd->getChildType(info.compMethodInfo->args.retTypeClass,
- &referentClassHandle);
- if (varTypeIsStruct(JITtype2varType(referentType)) &&
- (varDsc->GetStructHnd() != referentClassHandle))
- {
- // We are returning a byref to struct1; the method signature specifies return type as
- // byref
- // to struct2. struct1 and struct2 are different so we are "reinterpreting" the struct.
- // This may happen in, for example, System.Runtime.CompilerServices.Unsafe.As<TFrom,
- // TTo>.
- // We need to mark the source struct variable as having overlapping fields because its
- // fields may be accessed using field handles of a different type, which may confuse
- // optimizations, in particular, value numbering.
-
- JITDUMP("\nSetting lvOverlappingFields to true on V%02u because of struct "
- "reinterpretation\n",
- addrChild->AsLclVarCommon()->GetLclNum());
-
- varDsc->lvOverlappingFields = true;
- }
- }
- }
- }
-
 #ifdef DEBUG
  if (verbose)
  {

src/coreclr/jit/lclmorph.cpp

@@ -210,7 +210,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
  // Arguments:
  // val - the input value
  // field - the FIELD node that uses the input address value
- // fieldSeqStore - the compiler's field sequence store
+ // compiler - the compiler instance
  //
  // Return Value:
  // `true` if the value was consumed. `false` if the input value
@@ -224,8 +224,9 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
  // if the offset overflows then location is not representable, must escape
  // - UNKNOWN => UNKNOWN
  //
- bool Field(Value& val, GenTreeField* field, FieldSeqStore* fieldSeqStore)
+ bool Field(Value& val, GenTreeField* field, Compiler* compiler)
  {
+
  assert(!IsLocation() && !IsAddress());
 
  if (val.IsLocation())
@@ -246,14 +247,80 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
  m_lclNum = val.m_lclNum;
  m_offset = newOffset.Value();
 
+ bool haveCorrectFieldForVN;
  if (field->gtFldMayOverlap)
  {
- m_fieldSeq = FieldSeqStore::NotAField();
+ haveCorrectFieldForVN = false;
  }
  else
  {
+ LclVarDsc* varDsc = compiler->lvaGetDesc(m_lclNum);
+ if (!varTypeIsStruct(varDsc))
+ {
+ haveCorrectFieldForVN = false;
+ }
+ else if (FieldSeqStore::IsPseudoField(field->gtFldHnd))
+ {
+ assert(compiler->compObjectStackAllocation());
+ // We use PseudoFields when accessing stack allocated classes.
+ haveCorrectFieldForVN = false;
+ }
+ else if (val.m_fieldSeq == nullptr)
+ {
+
+ CORINFO_CLASS_HANDLE clsHnd = varDsc->GetStructHnd();
+ // If the answer is no we are probably accessing a canon type with a non-canon fldHnd,
+ // currently it could happen in crossgen2 scenario where VM distinguishes class<canon>._field
+ // from class<not-canon-ref-type>._field.
+ haveCorrectFieldForVN =
+ compiler->info.compCompHnd->doesFieldBelongToClass(field->gtFldHnd, clsHnd);
+ }
+ else
+ {
+ FieldSeqNode* lastSeqNode = val.m_fieldSeq->GetTail();
+ assert(lastSeqNode != nullptr);
+ if (lastSeqNode->IsPseudoField() || lastSeqNode == FieldSeqStore::NotAField())
+ {
+ haveCorrectFieldForVN = false;
+ }
+ else
+ {
+ CORINFO_FIELD_HANDLE lastFieldBeforeTheCurrent = lastSeqNode->GetFieldHandle();
+
+ CORINFO_CLASS_HANDLE clsHnd;
+ CorInfoType fieldCorType =
+ compiler->info.compCompHnd->getFieldType(lastFieldBeforeTheCurrent, &clsHnd);
+ if (fieldCorType != CORINFO_TYPE_VALUECLASS)
+ {
+ // For example, System.IntPtr:ToInt64, when inlined, creates trees like
+ // * FIELD long _value
+ // \--* ADDR byref
+ // \--* FIELD long Information
+ // \--* ADDR byref
+ // \--* LCL_VAR struct<Interop+NtDll+IO_STATUS_BLOCK, 16> V08 tmp7
+ haveCorrectFieldForVN = false;
+ }
+ else
+ {
+
+ haveCorrectFieldForVN =
+ compiler->info.compCompHnd->doesFieldBelongToClass(field->gtFldHnd, clsHnd);
+ noway_assert(haveCorrectFieldForVN);
+ }
+ }
+ }
+ }
+
+ if (haveCorrectFieldForVN)
+ {
+ FieldSeqStore* fieldSeqStore = compiler->GetFieldSeqStore();
  m_fieldSeq = fieldSeqStore->Append(val.m_fieldSeq, fieldSeqStore->CreateSingleton(field->gtFldHnd));
  }
+ else
+ {
+ m_fieldSeq = FieldSeqStore::NotAField();
+ JITDUMP("Setting NotAField for [%06u],\n", compiler->dspTreeID(field));
+ }
  }
 
  INDEBUG(val.Consume();)
@@ -463,7 +530,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
  assert(TopValue(1).Node() == node);
  assert(TopValue(0).Node() == node->AsField()->gtFldObj);
 
- if (!TopValue(1).Field(TopValue(0), node->AsField(), m_compiler->GetFieldSeqStore()))
+ if (!TopValue(1).Field(TopValue(0), node->AsField(), m_compiler))
  {
  // Either the address comes from a location value (e.g. FIELD(IND(...)))
  // or the field offset has overflowed.