From b648fe5f74701aef55d3036f6806e850f94553c5 Mon Sep 17 00:00:00 2001 From: Dwayne Robinson Date: Wed, 23 Sep 2020 01:56:19 -0700 Subject: [PATCH] ORT DirectML EP for Iron release, ONNX 1.5 (part 2) (#5263) * Merged PR 5195856: Fix broken cases of zero size tensors in Cast/Reduce MaskRCNN failed when `Cast` tried to execute `Xor` with emptiness (zero in dimensions). This is perfectly legal and should be treated as a nop. Ultimately DML itself should treat this case as a nop, just like how C's `memcpy` treats 0 count as a nop, but I'm just addressing it in ORT now, as enabling it in DML would impact more operators to be consistent (probably should incrementally add a flag to tensor validation so operators can be opted in gradually). Corresponding WindowsAI PR: https://microsoft.visualstudio.com/WindowsAI/_git/WindowsAI/pullrequest/5195850 Related work items: #27469839, #28761382 * Merged PR 5201369: Remove copy of initializers added in DMLXP refactor When used in ORT, a common method shouldn't copy and return initializer data Related work items: #29514403 Co-authored-by: Justin Stoecker Co-authored-by: Jeff Bloomfield --- .../src/FusedGraphKernel.cpp | 3 +- .../src/GraphKernelHelper.cpp | 31 +++++++++++++------ .../src/GraphKernelHelper.h | 3 +- .../src/GraphPartitioner.cpp | 2 +- .../src/Operators/DmlOperator.cpp | 5 +++ .../src/Operators/DmlOperator.h | 2 ++ .../src/Operators/DmlOperatorCast.cpp | 4 +-- .../src/Operators/DmlOperatorTopk.cpp | 5 ++- .../src/Operators/OperatorUtility.cpp | 2 +- .../dml/DmlExecutionProvider/src/TensorDesc.h | 2 +- 10 files changed, 41 insertions(+), 18 deletions(-) diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/FusedGraphKernel.cpp b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/FusedGraphKernel.cpp index f3f4caab3f..1bacde4134 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/FusedGraphKernel.cpp +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/FusedGraphKernel.cpp @@ -89,7 +89,7 @@ namespace Dml m_nonOwnedGraphInputsFromInitializers.resize(graphInputCount); std::vector> initializeResourceRefs; - GraphKernelHelper::PopulateInputBindings( + GraphKernelHelper::ProcessInputData( m_provider.Get(), m_winmlProvider.Get(), m_inputsConstant, @@ -101,6 +101,7 @@ namespace Dml initInputResources, m_nonOwnedGraphInputsFromInitializers, initializeResourceRefs, + nullptr, transferredInitializerMap); DML_GRAPH_DESC dmlGraphDesc = {}; diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphKernelHelper.cpp b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphKernelHelper.cpp index fa9792b08c..f6aa742ce0 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphKernelHelper.cpp +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphKernelHelper.cpp @@ -133,7 +133,7 @@ namespace GraphKernelHelper return true; }; - std::vector> PopulateInputBindings( + void ProcessInputData( Dml::IExecutionProvider* provider, IWinmlExecutionProvider* winmlProvider, const std::vector& inputsConstant, @@ -145,10 +145,9 @@ namespace GraphKernelHelper _Out_ std::vector>& initInputResources, _Out_ std::vector>& nonOwnedGraphInputsFromInitializers, _Out_ std::vector>& initializeResourceRefs, + _Out_opt_ std::vector>* inputRawData, _Inout_ std::unordered_map& transferredInitializerMap) { - std::vector> inputRawData; - const uint32_t graphInputCount = kernelInfo.GetInputCount(); // Determine the last input which uses an initializer, so initializers can be freed incrementally // while processing each input in order. @@ -166,6 +165,7 @@ namespace GraphKernelHelper for (const DML_INPUT_GRAPH_EDGE_DESC& edge : graphDesc.inputEdges) { inputsUsed[edge.GraphInputIndex] = true; } + for (uint32_t i = 0; i < initInputBindings.size(); i++) { // If the input isn't actually used by the graph, nothing ever needs to be bound (either for @@ -173,7 +173,12 @@ namespace GraphKernelHelper if (!inputsUsed[i]) { transferredInitializerMap.erase(fusedNodeInputDefs[i]->Name()); - inputRawData.push_back(std::vector()); + + if (inputRawData) + { + inputRawData->push_back(std::vector()); + } + continue; } @@ -202,7 +207,10 @@ namespace GraphKernelHelper // Tensor sizes in DML must be a multiple of 4 bytes large. tensorByteSize = AlignToPow2(tensorByteSize, 4); - inputRawData.push_back(std::vector(tensorPtr, tensorPtr + tensorByteSize)); + if (inputRawData) + { + inputRawData->push_back(std::vector(tensorPtr, tensorPtr + tensorByteSize)); + } if (!inputsConstant[i]) { @@ -243,8 +251,12 @@ namespace GraphKernelHelper THROW_HR_IF(E_UNEXPECTED, !kernelInfo.TryGetConstantInput(i, &inputTensor)); const std::byte* tensorData = reinterpret_cast(inputTensor->DataRaw()); - inputRawData.push_back( - std::vector(tensorData, tensorData + inputTensor->SizeInBytes())); + + if (inputRawData) + { + inputRawData->push_back( + std::vector(tensorData, tensorData + inputTensor->SizeInBytes())); + } uint64_t allocId; UnwrapTensor(winmlProvider, inputTensor, &initInputBindings[i].Buffer, &allocId); @@ -253,15 +265,14 @@ namespace GraphKernelHelper initInputBindings[i].Buffer->Release(); // Avoid holding an additional reference initInputResources.push_back(initInputBindings[i].Buffer); } - else + else if (inputRawData) { - inputRawData.push_back(std::vector()); + inputRawData->push_back(std::vector()); } } // All initializers should have been consumed and freed above assert(transferredInitializerMap.empty()); - return inputRawData; } void ConvertGraphDesc( diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphKernelHelper.h b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphKernelHelper.h index b1b2e87cf8..6f271612c6 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphKernelHelper.h +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphKernelHelper.h @@ -39,7 +39,7 @@ namespace GraphKernelHelper const onnxruntime::ConstPointerContainer>& fusedNodeInputDefs, const std::unordered_map& transferredInitializerMap); - std::vector> PopulateInputBindings( + void ProcessInputData( Dml::IExecutionProvider* provider, IWinmlExecutionProvider* winmlProvider, const std::vector& inputsConstant, @@ -51,6 +51,7 @@ namespace GraphKernelHelper _Out_ std::vector>& initInputResources, _Out_ std::vector>& nonOwnedGraphInputsFromInitializers, _Out_ std::vector>& initializeResourceRefs, + _Out_opt_ std::vector>* inputRawData, _Inout_ std::unordered_map& transferredInitializerMap); void ConvertGraphDesc( diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphPartitioner.cpp b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphPartitioner.cpp index a139b5fdda..1340ff7c8a 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphPartitioner.cpp +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/GraphPartitioner.cpp @@ -483,7 +483,7 @@ namespace Dml (requiredInputCount == std::nullopt || *requiredInputCount == node.InputDefs().size())) { *isDmlGraphNode = true; - graphNodeProperty.first->second.internalRegInfo = internalRegInfo; + graphNodeProperty.first->second.internalRegInfo = internalRegInfo; } } } diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperator.cpp b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperator.cpp index 2308766d71..085c55637e 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperator.cpp +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperator.cpp @@ -336,6 +336,11 @@ namespace Dml ComPtr DmlOperator::InitializeZeroInt64Tensor(uint64_t tensorSizeInBytes) { + if (tensorSizeInBytes == 0) + { + return nullptr; // No work to do. + } + // This fun little solution uses DML's element-wise shader with XOR to zero the memory of the passed-in // tensor. This requires that the tensor's memory has been initialized (i.e. raw_mutable_data has been // called, and there is a size to the tensor). The tensor is XOR'd with itself to produce zeros, diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperator.h b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperator.h index dedfb41e71..1a8dda0e0e 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperator.h +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperator.h @@ -79,6 +79,8 @@ namespace Dml // will read the full 64-bit values. This means it is necessary to zero out the memory to ensure there // are no uninitialized values in the upper 32-bit portion of the tensor memory. // + // It returns nullptr if there is no work to do (0 bytes). + // ComPtr InitializeZeroInt64Tensor(uint64_t tensorSizeInBytes); void ExecuteZeroInt64Tensor(IDMLCompiledOperator* compiledOperator, IMLOperatorTensor* tensor); diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperatorCast.cpp b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperatorCast.cpp index fc43e96438..51f93d8683 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperatorCast.cpp +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperatorCast.cpp @@ -42,9 +42,9 @@ public: std::vector outputTensors = GetOutputTensorsForExecute(kernelContext); // Zero the output tensor's memory for 64-bit integer emulation with strides. - if (m_toDataType == MLOperatorTensorDataType::UInt64 || m_toDataType == MLOperatorTensorDataType::Int64) + if (m_zeroOperator) { - assert(m_zeroOperator); + assert(m_toDataType == MLOperatorTensorDataType::UInt64 || m_toDataType == MLOperatorTensorDataType::Int64); ExecuteZeroInt64Tensor(m_zeroOperator.Get(), outputTensors[0]); } diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperatorTopk.cpp b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperatorTopk.cpp index adcc5b07f8..50cfc1075e 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperatorTopk.cpp +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/DmlOperatorTopk.cpp @@ -58,7 +58,10 @@ public: std::vector inputTensors = GetInputTensorsForExecute(kernelContext); std::vector outputTensors = GetOutputTensorsForExecute(kernelContext); - ExecuteZeroInt64Tensor(m_zeroOperator.Get(), outputTensors[1]); + if (m_zeroOperator) + { + ExecuteZeroInt64Tensor(m_zeroOperator.Get(), outputTensors[1]); + } THROW_IF_FAILED(m_executionProvider->ExecuteOperator( m_compiledOperator.Get(), diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/OperatorUtility.cpp b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/OperatorUtility.cpp index fe497bec34..afbe678b60 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/OperatorUtility.cpp +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/Operators/OperatorUtility.cpp @@ -215,7 +215,7 @@ namespace Dml return std::nullopt; } - // All fused ops just have "Fused" prepended to their name + // All fused ops just have "Fused" prepended to their name (e.g. "Conv" -> "FusedConv"). std::string fusedOpType = std::string("Fused").append(candidateOpType); return FusedOpProperties{ std::move(fusedOpType), onnxruntime::kMSDmlDomain }; diff --git a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/TensorDesc.h b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/TensorDesc.h index e9b7d97c48..d0d4a5c2b8 100644 --- a/onnxruntime/core/providers/dml/DmlExecutionProvider/src/TensorDesc.h +++ b/onnxruntime/core/providers/dml/DmlExecutionProvider/src/TensorDesc.h @@ -46,7 +46,7 @@ namespace Dml gsl::span GetSizes() const { return { m_sizes, m_sizes + m_bufferTensorDesc.DimensionCount }; } gsl::span GetStrides() const; - inline UINT64 GetBufferSizeInBytes() const + inline uint64_t GetBufferSizeInBytes() const { assert(m_tensorType == DML_TENSOR_TYPE_BUFFER); return m_bufferTensorDesc.TotalTensorSizeInBytes;