Print_formatter refactor

Change-Id: Icb03697281b009c853d91a63d5d21ffcde545a8f
Signed-off-by: Maciej Plewka <[email protected]>

Author: MaciejPlewka

Jenkinsfile

@@ -1,5 +1,5 @@
 #!groovy
 dependenciesRevision='e299ec97c8d0d14d604a94606ce11c7498e30a76-1244'
 strategy='EQUAL'
-allowedCD=276
+allowedCD=275
 allowedF=4

runtime/program/kernel_info.cpp

@@ -199,9 +199,6 @@ void WorkSizeInfo::checkRatio(const size_t workItems[3]) {
 KernelInfo::~KernelInfo() {
     kernelArgInfo.clear();
 
-    for (auto &stringData : patchInfo.stringDataMap) {
-        delete[] stringData.second.pStringData;
-    }
     patchInfo.stringDataMap.clear();
     delete[] crossThreadData;
 }
@@ -376,14 +373,9 @@ void KernelInfo::storePatchToken(const SPatchAllocateStatelessDefaultDeviceQueue
 
 void KernelInfo::storePatchToken(const SPatchString *pStringArg) {
     uint32_t stringIndex = pStringArg->Index;
-    PrintfStringInfo printfStringInfo;
-    printfStringInfo.SizeInBytes = pStringArg->StringSize;
-    if (printfStringInfo.SizeInBytes) {
-        printfStringInfo.pStringData = new char[printfStringInfo.SizeInBytes];
-        if (printfStringInfo.pStringData != nullptr) {
-            memcpy_s(printfStringInfo.pStringData, printfStringInfo.SizeInBytes, (cl_char *)pStringArg + sizeof(SPatchString), printfStringInfo.SizeInBytes);
-            patchInfo.stringDataMap.insert(std::pair<uint32_t, PrintfStringInfo>(stringIndex, printfStringInfo));
-        }
+    if (pStringArg->StringSize > 0) {
+        const char *stringData = reinterpret_cast<const char *>(pStringArg + 1);
+        patchInfo.stringDataMap.emplace(stringIndex, std::string(stringData, stringData + pStringArg->StringSize));
     }
 }
 
@@ -404,11 +396,6 @@ void KernelInfo::storePatchToken(const SPatchAllocateSystemThreadSurface *pSyste
     patchInfo.pAllocateSystemThreadSurface = pSystemThreadSurface;
 }
 
-const char *KernelInfo::queryPrintfString(uint32_t index) const {
-    auto printfInfo = patchInfo.stringDataMap.find(index);
-    return printfInfo == patchInfo.stringDataMap.end() ? nullptr : printfInfo->second.pStringData;
-}
-
 cl_int KernelInfo::resolveKernelInfo() {
     cl_int retVal = CL_SUCCESS;
     std::unordered_map<std::string, uint32_t>::iterator iterUint;

runtime/program/kernel_info.h

@@ -163,8 +163,6 @@ struct KernelInfo {
 
     void storeKernelArgPatchInfo(uint32_t argNum, uint32_t dataSize, uint32_t crossthreadOffset, uint32_t sourceOffset, uint32_t offsetSSH);
 
-    const char *queryPrintfString(uint32_t index) const;
-
     size_t getSamplerStateArrayCount() const;
     size_t getSamplerStateArraySize(const HardwareInfo &hwInfo) const;
     size_t getBorderColorStateSize() const;

runtime/program/patch_info.h

@@ -10,6 +10,8 @@
 #include "patch_list.h"
 
 #include <map>
+#include <string>
+#include <unordered_map>
 #include <vector>
 
 namespace NEO {
@@ -44,11 +46,6 @@ using iOpenCL::SPatchString;
 using iOpenCL::SPatchThreadPayload;
 using iOpenCL::SProgramBinaryHeader;
 
-typedef struct TagPrintfStringInfo {
-    size_t SizeInBytes;
-    char *pStringData;
-} PrintfStringInfo, *PPrintfStringInfo;
-
 struct PatchInfo {
     const SPatchMediaInterfaceDescriptorLoad *interfaceDescriptorDataLoad = nullptr;
     const SPatchAllocateLocalSurface *localsurface = nullptr;
@@ -74,7 +71,7 @@ struct PatchInfo {
     const SPatchAllocateStatelessEventPoolSurface *pAllocateStatelessEventPoolSurface = nullptr;
     const SPatchAllocateStatelessDefaultDeviceQueueSurface *pAllocateStatelessDefaultDeviceQueueSurface = nullptr;
     const SPatchAllocateSystemThreadSurface *pAllocateSystemThreadSurface = nullptr;
-    ::std::map<uint32_t, PrintfStringInfo> stringDataMap;
+    ::std::unordered_map<uint32_t, std::string> stringDataMap;
     ::std::vector<const SPatchKernelArgumentInfo *> kernelArgumentInfo;
 
     PatchInfo() {

runtime/program/print_formatter.cpp

@@ -14,27 +14,24 @@
 
 namespace NEO {
 
-PrintFormatter::PrintFormatter(Kernel &kernelArg, GraphicsAllocation &dataArg) : kernel(kernelArg),
-                                                                                 data(dataArg),
-                                                                                 buffer(nullptr),
-                                                                                 bufferSize(0),
-                                                                                 offset(0) {
+PrintFormatter::PrintFormatter(const uint8_t *printfOutputBuffer, uint32_t printfOutputBufferMaxSize,
+                               bool using32BitPointers, const StringMap &stringLiteralMap)
+    : printfOutputBuffer(printfOutputBuffer),
+      printfOutputBufferSize(printfOutputBufferMaxSize),
+      stringLiteralMap(stringLiteralMap),
+      using32BitPointers(using32BitPointers) {
 }
 
 void PrintFormatter::printKernelOutput(const std::function<void(char *)> &print) {
-    offset = 0;
-    buffer = reinterpret_cast<uint8_t *>(data.getUnderlyingBuffer());
+    currentOffset = 0;
 
-    // first 4 bytes of the buffer store it's own size
-    // before reading it size needs to be set to 4 because read() checks bounds and would fail if bufferSize was 0
-    bufferSize = 4;
-    read(&bufferSize);
+    // first 4 bytes of the buffer store the actual size of data that was written by printf from within EUs
+    read(&printfOutputBufferSize);
 
     uint32_t stringIndex = 0;
-
-    while (offset + 4 <= bufferSize) {
+    while (currentOffset + 4 <= printfOutputBufferSize) {
         read(&stringIndex);
-        const char *formatString = kernel.getKernelInfo().queryPrintfString(stringIndex);
+        const char *formatString = queryPrintfString(stringIndex);
         if (formatString != nullptr) {
             printString(formatString, print);
         }
@@ -146,7 +143,7 @@ size_t PrintFormatter::printStringToken(char *output, size_t size, const char *f
     read(&type);
     read(&index);
     if (type == static_cast<int>(PRINTF_DATA_TYPE::STRING)) {
-        return simple_sprintf(output, size, formatString, kernel.getKernelInfo().queryPrintfString(index));
+        return simple_sprintf(output, size, formatString, queryPrintfString(index));
     } else {
         return simple_sprintf(output, size, formatString, 0);
     }
@@ -156,7 +153,7 @@ size_t PrintFormatter::printPointerToken(char *output, size_t size, const char *
     uint64_t value = {0};
     read(&value);
 
-    if (kernel.is32Bit()) {
+    if (using32BitPointers) {
         value &= 0x00000000FFFFFFFF;
     }
 
@@ -196,4 +193,10 @@ bool PrintFormatter::isConversionSpecifier(char c) {
         return false;
     }
 }
+
+const char *PrintFormatter::queryPrintfString(uint32_t index) const {
+    auto stringEntry = stringLiteralMap.find(index);
+    return stringEntry == stringLiteralMap.end() ? nullptr : stringEntry->second.c_str();
+}
+
 } // namespace NEO

runtime/program/print_formatter.h

@@ -15,11 +15,15 @@
 #include <cctype>
 #include <cstdint>
 #include <functional>
+#include <string>
+#include <unordered_map>
 
 extern int memcpy_s(void *dst, size_t destSize, const void *src, size_t count);
 
 namespace NEO {
 
+using StringMap = std::unordered_map<uint32_t, std::string>;
+
 enum class PRINTF_DATA_TYPE : int {
     INVALID,
     BYTE,
@@ -40,12 +44,14 @@ enum class PRINTF_DATA_TYPE : int {
 
 class PrintFormatter {
   public:
-    PrintFormatter(Kernel &kernelArg, GraphicsAllocation &dataArg);
+    PrintFormatter(const uint8_t *printfOutputBuffer, uint32_t printfOutputBufferMaxSize,
+                   bool using32BitPointers, const StringMap &stringLiteralMap);
     void printKernelOutput(const std::function<void(char *)> &print = [](char *str) { printToSTDOUT(str); });
 
     static const size_t maxPrintfOutputLength = 1024;
 
   protected:
+    const char *queryPrintfString(uint32_t index) const;
     void printString(const char *formatString, const std::function<void(char *)> &print);
     size_t printToken(char *output, size_t size, const char *formatString);
     size_t printStringToken(char *output, size_t size, const char *formatString);
@@ -58,15 +64,15 @@ class PrintFormatter {
 
     template <class T>
     bool read(T *value) {
-        if (offset + sizeof(T) <= bufferSize) {
-            auto srcPtr = reinterpret_cast<T *>(buffer + offset);
+        if (currentOffset + sizeof(T) <= printfOutputBufferSize) {
+            auto srcPtr = reinterpret_cast<const T *>(printfOutputBuffer + currentOffset);
 
             if (isAligned(srcPtr)) {
                 *value = *srcPtr;
             } else {
-                memcpy_s(value, bufferSize - offset, srcPtr, sizeof(T));
+                memcpy_s(value, printfOutputBufferSize - currentOffset, srcPtr, sizeof(T));
             }
-            offset += sizeof(T);
+            currentOffset += sizeof(T);
             return true;
         } else {
             return false;
@@ -101,17 +107,18 @@ class PrintFormatter {
         }
 
         if (sizeof(T) < 4) {
-            offset += (4 - sizeof(T)) * valueCount;
+            currentOffset += (4 - sizeof(T)) * valueCount;
         }
 
         return charactersPrinted;
     }
 
-    Kernel &kernel;
-    GraphicsAllocation &data;
+    const uint8_t *printfOutputBuffer = nullptr; // buffer extracted from the kernel, contains values to be printed
+    uint32_t printfOutputBufferSize = 0;         // size of the data contained in the buffer
+
+    const StringMap &stringLiteralMap;
+    bool using32BitPointers = false;
 
-    uint8_t *buffer;     // buffer extracted from the kernel, contains values to be printed
-    uint32_t bufferSize; // size of the data contained in the buffer
-    uint32_t offset;     // current position in currently parsed buffer
+    uint32_t currentOffset = 0; // current position in currently parsed buffer
 };
 }; // namespace NEO

runtime/program/printf_handler.cpp

@@ -58,7 +58,8 @@ void PrintfHandler::makeResident(CommandStreamReceiver &commandStreamReceiver) {
 }
 
 void PrintfHandler::printEnqueueOutput() {
-    PrintFormatter printFormatter(*kernel, *printfSurface);
+    PrintFormatter printFormatter(reinterpret_cast<const uint8_t *>(printfSurface->getUnderlyingBuffer()), static_cast<uint32_t>(printfSurface->getUnderlyingBufferSize()),
+                                  kernel->is32Bit(), kernel->getKernelInfo().patchInfo.stringDataMap);
     printFormatter.printKernelOutput();
 }
 } // namespace NEO

unit_tests/command_queue/enqueue_kernel_2_tests.cpp

@@ -610,14 +610,9 @@ HWTEST_P(EnqueueKernelPrintfTest, GivenKernelWithPrintfBlockedByEventWhenEventUn
 
         auto crossThreadData = reinterpret_cast<uint64_t *>(mockKernel.mockKernel->getCrossThreadData());
 
-        char *testString = new char[sizeof("test")];
-        strcpy_s(testString, sizeof("test"), "test");
+        std::string testString = "test";
 
-        PrintfStringInfo printfStringInfo;
-        printfStringInfo.SizeInBytes = sizeof("test");
-        printfStringInfo.pStringData = testString;
-
-        mockKernel.kernelInfo.patchInfo.stringDataMap.insert(std::make_pair(0, printfStringInfo));
+        mockKernel.kernelInfo.patchInfo.stringDataMap.insert(std::make_pair(0, testString));
 
         cl_uint workDim = 1;
         size_t globalWorkOffset[3] = {0, 0, 0};

unit_tests/event/event_tests.cpp

@@ -548,19 +548,13 @@ TEST_F(InternalsEventTest, givenBlockedKernelWithPrintfWhenSubmittedThenPrintOut
     pPrintfSurface->DataParamOffset = 0;
     pPrintfSurface->DataParamSize = 8;
 
-    char *testString = new char[sizeof("test")];
-
-    strcpy_s(testString, sizeof("test"), "test");
-
-    PrintfStringInfo printfStringInfo;
-    printfStringInfo.SizeInBytes = sizeof("test");
-    printfStringInfo.pStringData = testString;
+    std::string testString = "test";
 
     MockKernelWithInternals mockKernelWithInternals(*pDevice);
     auto pKernel = mockKernelWithInternals.mockKernel;
     KernelInfo *kernelInfo = const_cast<KernelInfo *>(&pKernel->getKernelInfo());
     kernelInfo->patchInfo.pAllocateStatelessPrintfSurface = pPrintfSurface;
-    kernelInfo->patchInfo.stringDataMap.insert(std::make_pair(0, printfStringInfo));
+    kernelInfo->patchInfo.stringDataMap.insert(std::make_pair(0, testString));
     uint64_t crossThread[10];
     pKernel->setCrossThreadData(&crossThread, sizeof(uint64_t) * 8);
 

unit_tests/program/kernel_data.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2018 Intel Corporation
+ * Copyright (C) 2017-2019 Intel Corporation
  *
  * SPDX-License-Identifier: MIT
  *
@@ -123,7 +123,7 @@ TEST_F(KernelDataTest, PrintfString) {
 
     buildAndDecode();
 
-    EXPECT_EQ_VAL(0, strcmp(stringValue, pKernelInfo->queryPrintfString(0)));
+    EXPECT_EQ_VAL(0, strcmp(stringValue, pKernelInfo->patchInfo.stringDataMap.find(0)->second.c_str()));
     delete[] pPrintfString;
 }