SPMI: Implement fallback schema layout (#78594)
authorJakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
Mon, 21 Nov 2022 15:30:19 +0000 (16:30 +0100)
committerGitHub <noreply@github.com>
Mon, 21 Nov 2022 15:30:19 +0000 (16:30 +0100)
Fallback to doing actual layout of the schema data when the schema does
not match the recorded schema. This ensures replays are still consistent
with the recording in cases where the JIT and environment variables
match, but that we can still succeed a replay on changes to these.

Fix #74718

src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp
src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h

index 1b062b8..b93c16a 100644 (file)
@@ -5410,8 +5410,26 @@ void MethodContext::recAllocPgoInstrumentationBySchema(
 
     Agnostic_AllocPgoInstrumentationBySchema value;
 
-    // NOTE: we store the `*pInstrumentationData` address, but not any data at that address. This makes sense, because when this is called,
-    // there is no data at the address. The JIT won't read/write the data, but only generate code to write to the buffer.
+    // PGO schema data poses a couple of challenges from SPMI's perspective:
+    //
+    // First of, the schema requested from the JIT can reasonably be considered
+    // a compile result that might change with different JIT versions and/or
+    // environment variables set. So that means SPMI should be able to handle
+    // seeing a different schema from recording time.
+    //
+    // On the other hand, the code generated by the JIT will refer to addresses
+    // based on pInstrumentationData and the offsets this function assigns in
+    // pSchema. That means this function should be consistent with the recorded
+    // EE in how it does layout, to ensure replay is going to match recording
+    // time.
+    //
+    // To handle these issues we store the recorded result of layout for the
+    // schema and use that if the JIT's requested schema matches it. Otherwise
+    // we use our own layout algorithm for the schema. This layout algorithm is
+    // probably not going to match the EE's algorithm exactly, but that's not
+    // problematic since a different schema likely means there are going to be
+    // diffs anyway.
+
     value.instrumentationDataAddress = CastPointer(*pInstrumentationData);
 
     // For the schema, note that we record *after* calling the VM API. Thus, it has already filled in the `Offset` fields.
@@ -5459,51 +5477,125 @@ void MethodContext::dmpAllocPgoInstrumentationBySchema(DWORDLONG key, const Agno
     printf("}");
 }
 
+
+// The following are from <pgo_formatprocessing.h> which we can't include, with
+// a few SPMI alterations (related to "target pointers").
+static ICorJitInfo::PgoInstrumentationKind operator&(ICorJitInfo::PgoInstrumentationKind a, ICorJitInfo::PgoInstrumentationKind b)
+{
+    return static_cast<ICorJitInfo::PgoInstrumentationKind>(static_cast<int>(a) & static_cast<int>(b));
+}
+
+static unsigned InstrumentationKindToSize(ICorJitInfo::PgoInstrumentationKind kind)
+{
+    switch(kind & ICorJitInfo::PgoInstrumentationKind::MarshalMask)
+    {
+        case ICorJitInfo::PgoInstrumentationKind::None:
+            return 0;
+        case ICorJitInfo::PgoInstrumentationKind::FourByte:
+            return 4;
+        case ICorJitInfo::PgoInstrumentationKind::EightByte:
+            return 8;
+        case ICorJitInfo::PgoInstrumentationKind::TypeHandle:
+        case ICorJitInfo::PgoInstrumentationKind::MethodHandle:
+            return static_cast<unsigned>(SpmiTargetPointerSize());
+        default:
+            LogError("Unexpected pgo schema data size (kind = %d)", kind);
+            return 0;
+    }
+}
+
+static unsigned InstrumentationKindToAlignment(ICorJitInfo::PgoInstrumentationKind kind)
+{
+    switch(kind & ICorJitInfo::PgoInstrumentationKind::AlignMask)
+    {
+        case ICorJitInfo::PgoInstrumentationKind::Align4Byte:
+            return 4;
+        case ICorJitInfo::PgoInstrumentationKind::Align8Byte:
+            return 8;
+        case ICorJitInfo::PgoInstrumentationKind::AlignPointer:
+            return static_cast<unsigned>(SpmiTargetPointerSize());
+        default:
+            return InstrumentationKindToSize(kind);
+    }
+}
+
+// End of pseudo-include
+
 HRESULT MethodContext::repAllocPgoInstrumentationBySchema(
     CORINFO_METHOD_HANDLE ftnHnd,
     ICorJitInfo::PgoInstrumentationSchema* pSchema,
     UINT32 countSchemaItems,
     BYTE** pInstrumentationData)
 {
+    if (repAllocPgoInstrumentationBySchemaRecorded(ftnHnd, pSchema, countSchemaItems, pInstrumentationData))
+    {
+        // Matching case: return same layout as recorded EE.
+        return S_OK;
+    }
+
+    // Do our own layout.
+    unsigned maxAlignment = 1;
+    unsigned offset = 0;
+    for (UINT32 i = 0; i < countSchemaItems; i++)
+    {
+        unsigned size = InstrumentationKindToSize(pSchema[i].InstrumentationKind) * pSchema[i].Count;
+        if (size != 0)
+        {
+            unsigned alignment = InstrumentationKindToAlignment(pSchema[i].InstrumentationKind);
+            maxAlignment = max(maxAlignment, alignment);
+            offset = AlignUp(offset, alignment);
+            pSchema[i].Offset = offset;
+        }
+
+        offset += size;
+    }
+
+    // We assume JIT does not write or read from this buffer, only generate
+    // code that uses it.
+    *pInstrumentationData = (BYTE*)intptr_t(AlignDown(0x12345678u, maxAlignment));
+    return S_OK;
+}
+
+// Allocate PGO instrumentation by schema in the same way that the original EE did if the schema matches.
+// See comment in recAllocPgoInstrumentationBySchema.
+// The schema may be partially allocated even if this function returns false.
+bool MethodContext::repAllocPgoInstrumentationBySchemaRecorded(
+    CORINFO_METHOD_HANDLE ftnHnd,
+    ICorJitInfo::PgoInstrumentationSchema* pSchema,
+    UINT32 countSchemaItems,
+    BYTE** pInstrumentationData)
+{
+    if (AllocPgoInstrumentationBySchema == nullptr)
+        return false;
+
     DWORDLONG key = CastHandle(ftnHnd);
-    AssertMapAndKeyExist(AllocPgoInstrumentationBySchema, key, ": key %016llX", key);
+    int index = AllocPgoInstrumentationBySchema->GetIndex(key);
+    if (index == -1)
+        return false;
 
-    Agnostic_AllocPgoInstrumentationBySchema value = AllocPgoInstrumentationBySchema->Get(key);
+    Agnostic_AllocPgoInstrumentationBySchema value = AllocPgoInstrumentationBySchema->GetItem(index);
     DEBUG_REP(dmpAllocPgoInstrumentationBySchema(key, value));
 
     if (value.countSchemaItems != countSchemaItems)
-    {
-        LogError("AllocPgoInstrumentationBySchema mismatch: countSchemaItems record %d, replay %d", value.countSchemaItems, countSchemaItems);
-    }
-
-    HRESULT result = (HRESULT)value.result;
+        return false;
 
     Agnostic_PgoInstrumentationSchema* pAgnosticSchema = (Agnostic_PgoInstrumentationSchema*)AllocPgoInstrumentationBySchema->GetBuffer(value.schema_index);
-    for (UINT32 iSchema = 0; iSchema < countSchemaItems && iSchema < value.countSchemaItems; iSchema++)
+    for (UINT32 iSchema = 0; iSchema < value.countSchemaItems; iSchema++)
     {
         // Everything but `Offset` field is an IN argument, so verify it against what we stored (since we didn't use these
         // IN arguments as part of the key).
 
         if ((ICorJitInfo::PgoInstrumentationKind)pAgnosticSchema[iSchema].InstrumentationKind != pSchema[iSchema].InstrumentationKind)
-        {
-            LogError("AllocPgoInstrumentationBySchema mismatch: [%d].InstrumentationKind record %d, replay %d",
-                iSchema, pAgnosticSchema[iSchema].InstrumentationKind, (DWORD)pSchema[iSchema].InstrumentationKind);
-        }
+            return false;
+
         if ((int32_t)pAgnosticSchema[iSchema].ILOffset != pSchema[iSchema].ILOffset)
-        {
-            LogError("AllocPgoInstrumentationBySchema mismatch: [%d].ILOffset record %d, replay %d",
-                iSchema, pAgnosticSchema[iSchema].ILOffset, (DWORD)pSchema[iSchema].ILOffset);
-        }
+            return false;
+
         if ((int32_t)pAgnosticSchema[iSchema].Count != pSchema[iSchema].Count)
-        {
-            LogError("AllocPgoInstrumentationBySchema mismatch: [%d].Count record %d, replay %d",
-                iSchema, pAgnosticSchema[iSchema].Count, (DWORD)pSchema[iSchema].Count);
-        }
+            return false;
+
         if ((int32_t)pAgnosticSchema[iSchema].Other != pSchema[iSchema].Other)
-        {
-            LogError("AllocPgoInstrumentationBySchema mismatch: [%d].Other record %d, replay %d",
-                iSchema, pAgnosticSchema[iSchema].Other, (DWORD)pSchema[iSchema].Other);
-        }
+            return false;
 
         pSchema[iSchema].Offset = (size_t)pAgnosticSchema[iSchema].Offset;
     }
@@ -5511,37 +5603,9 @@ HRESULT MethodContext::repAllocPgoInstrumentationBySchema(
     // We assume JIT does not write or read from this buffer, only generate
     // code that uses it.
     *pInstrumentationData = (BYTE*)value.instrumentationDataAddress;
-    return result;
-}
-
-// The following are from <pgo_formatprocessing.h> which we can't include.
-
-static ICorJitInfo::PgoInstrumentationKind operator&(ICorJitInfo::PgoInstrumentationKind a, ICorJitInfo::PgoInstrumentationKind b)
-{
-    return static_cast<ICorJitInfo::PgoInstrumentationKind>(static_cast<int>(a) & static_cast<int>(b));
-}
-
-static unsigned InstrumentationKindToSize(ICorJitInfo::PgoInstrumentationKind kind)
-{
-    switch(kind & ICorJitInfo::PgoInstrumentationKind::MarshalMask)
-    {
-        case ICorJitInfo::PgoInstrumentationKind::None:
-            return 0;
-        case ICorJitInfo::PgoInstrumentationKind::FourByte:
-            return 4;
-        case ICorJitInfo::PgoInstrumentationKind::EightByte:
-            return 8;
-        case ICorJitInfo::PgoInstrumentationKind::TypeHandle:
-        case ICorJitInfo::PgoInstrumentationKind::MethodHandle:
-            return sizeof(uintptr_t);
-        default:
-            LogError("Unexpected pgo schema data size (kind = %d)", kind);
-            return 0;
-    }
+    return true;
 }
 
-// End of pseudo-include
-
 void MethodContext::recGetPgoInstrumentationResults(CORINFO_METHOD_HANDLE ftnHnd,
                                                     ICorJitInfo::PgoInstrumentationSchema** pSchema,
                                                     UINT32* pCountSchemaItems,
index c0535de..b1ef506 100644 (file)
@@ -719,6 +719,7 @@ public:
     void recAllocPgoInstrumentationBySchema(CORINFO_METHOD_HANDLE ftnHnd, ICorJitInfo::PgoInstrumentationSchema* pSchema, UINT32 countSchemaItems, BYTE** pInstrumentationData, HRESULT result);
     void dmpAllocPgoInstrumentationBySchema(DWORDLONG key, const Agnostic_AllocPgoInstrumentationBySchema& value);
     HRESULT repAllocPgoInstrumentationBySchema(CORINFO_METHOD_HANDLE ftnHnd, ICorJitInfo::PgoInstrumentationSchema* pSchema, UINT32 countSchemaItems, BYTE** pInstrumentationData);
+    bool repAllocPgoInstrumentationBySchemaRecorded(CORINFO_METHOD_HANDLE ftnHnd, ICorJitInfo::PgoInstrumentationSchema* pSchema, UINT32 countSchemaItems, BYTE** pInstrumentationData);
 
     void recGetPgoInstrumentationResults(CORINFO_METHOD_HANDLE ftnHnd, ICorJitInfo::PgoInstrumentationSchema** pSchema, UINT32* pCountSchemaItems, BYTE** pInstrumentationData, ICorJitInfo::PgoSource* pPgoSource, HRESULT result);
     void dmpGetPgoInstrumentationResults(DWORDLONG key, const Agnostic_GetPgoInstrumentationResults& value);