Create and initialize Device object: the first PR for rewriting DML backend #287

mingmingtasd · 2022-08-03T03:23:01Z

PTAL, thanks! @bbernhar @fujunwei @miaobin

bbernhar · 2022-08-03T17:06:48Z

@mingmingtasd Could we please either rename the old backend (dml) or give the new backend another (temporary) name? That way, the git diffs will be accurate (= doesn't mix old and new). A plus, will also keep the bots green.

bbernhar

Thanks @mingmingtasd

src/webnn/native/dml/DeviceDML.cpp

src/webnn/native/dml/DeviceDML.h

src/webnn/native/dml/dml_platform.h

bbernhar · 2022-08-03T22:09:23Z

src/webnn/native/dml/DeviceDML.h

+        bool useDebugLayer = false;
+    };
+
+    class Device {


What do you think about replacing "Device" with something like "ExecutionContext"?

I'm pretty certain that this code originates from Dawn. In Dawn, a device object is called "logical" (meaning there is state attached to it). In D3D12, the device object is actually stateless. The reason Dawn used a logical device is because Vulkan required such abstraction. Since DML does not depend on Vulkan, we don't need to create your own fake-logical device like this. Instead, we really just need a "context" that flushes DML commands using a queue + device + recorder.

Yes, thanks, I agree to rename it, but there is a ContextDM.cpp, will we rename Device.cpp as ExecutionContextDML.cpp? Maybe a little confused...

Looks like we could re-use ContextDML for this. ExecutionContextDML could be a concrete implementation of ContextBase. We could construct a ExecutionContextDML by passing the D3D12 device, DML device, and D3D12 queue to it. A provider-factory, AdapterDML, would produce ExecutionContextDML (and creates the devices and queue). The Backend uses AdapterDML to produce this context.

OK, I deleted ExecutionContextDML and moved the

ComPtr<IDMLDevice> mDevice; ComPtr<ID3D12Device> mD3D12Device; ComPtr<IDMLCommandRecorder> mCommandRecorder; ComPtr<ID3D12CommandQueue> mCommandQueue; ComPtr<ID3D12CommandAllocator> mCommandAllocator; ComPtr<ID3D12GraphicsCommandList> mCommandList; ComPtr<IDXGIAdapter1> mAdapter;

into ContextDML.cpp, and let Backend use Adapter to create the context for DML.

I still like ExecutionContext - "context" sounds to vague to me. But I don't feel strongly about that, you decide.

mingmingtasd · 2022-08-04T01:35:42Z

@mingmingtasd Could we please either rename the old backend (dml) or give the new backend another (temporary) name? That way, the git diffs will be accurate (= doesn't mix old and new). A plus, will also keep the bots green.

OK, I prefer to add a new temporary backend. @bbernhar And we can replace the old dml backend with the new backend when all ready.

fujunwei · 2022-08-04T02:19:40Z

@mingmingtasd Could we please either rename the old backend (dml) or give the new backend another (temporary) name? That way, the git diffs will be accurate (= doesn't mix old and new). A plus, will also keep the bots green.

OK, I prefer to add a new temporary backend. @bbernhar And we can replace the old dml backend with the new backend when all ready.

Add a temporary backend in main branch will confuse for the developer, you can draft a first PR to delete the DML backend implementation and disable the CI. Another approach is to rewrite the DML backend on your Repo that also can be reviewed by us.

mingmingtasd · 2022-08-04T02:56:39Z

@mingmingtasd Could we please either rename the old backend (dml) or give the new backend another (temporary) name? That way, the git diffs will be accurate (= doesn't mix old and new). A plus, will also keep the bots green.

OK, I prefer to add a new temporary backend. @bbernhar And we can replace the old dml backend with the new backend when all ready.

Add a temporary backend in main branch will confuse for the developer, you can draft a first PR to delete the DML backend implementation and disable the CI. Another approach is to rewrite the DML backend on your Repo that also can be reviewed by us.

OK, agree, thanks! @fujunwei @bbernhar

src/webnn/native/dml/UtilsDML.h

bbernhar · 2022-08-04T16:20:24Z

@fujunwei Seems fine to me too.

mingmingtasd · 2022-08-08T07:52:55Z

@miaobin Could you help enable some simple ops like Sigmoid firstly for testing? Then you can give more feedbacks when we are re-writing the DML backend. Thanks!

miaobin · 2022-08-09T07:15:41Z

@miaobin Could you help enable some simple ops like Sigmoid firstly for testing? Then you can give more feedbacks when we are re-writing the DML backend. Thanks!

Of course, I will write GraphDML based on the current PR. On one hand, I will move some content in Device.cpp to this layer, on the other hand, I will first implement add input, compile, compute and simple operators such as sigmoid.

src/webnn/native/dml/dml_platform.h

bbernhar

Looking good! Left some comments. Mostly regarding: recorder vs execution-context split. The others are minor.

src/webnn/native/dml/BackendDML.cpp

src/webnn/native/dml/BackendDML.h

bbernhar · 2022-08-16T22:44:50Z

src/webnn/native/dml/ContextDML.h

+
+        ComPtr<IDMLDevice> mDevice;
+        ComPtr<ID3D12Device> mD3D12Device;
+        ComPtr<IDMLCommandRecorder> mCommandRecorder;


What do you think about having CommandRecorderDML?

CommandRecorderDML would store theID3D12CommandAllocator and be stored within this context. Then everything that needs the recorder but not the execution context, eg. InitializeOperator, ExecuteOperator is moved to CommandRecorderDML. This keeps the logic of recording clearly and cleanly separated from execution. The context just asks for the recorded command-list from this instance to execute.

Do you mean move these member variables into

CommandRecorderDML{ ComPtr<IDMLDevice> mDevice; ComPtr<ID3D12Device> mD3D12Device; ComPtr<IDMLCommandRecorder> mCommandRecorder; ComPtr<ID3D12CommandQueue> mCommandQueue; ComPtr<ID3D12CommandAllocator> mCommandAllocator; ComPtr<ID3D12GraphicsCommandList> mCommandList; }

?

Yup, that's right.

OK, updated.

bbernhar · 2022-08-16T22:49:30Z

src/webnn/native/dml/ContextDML.cpp

+            }
+        }
+        WEBNN_RETURN_IF_FAILED(
+            D3D12CreateDevice(mAdapter.Get(), D3D_FEATURE_LEVEL_11_0, IID_PPV_ARGS(&mD3D12Device)));


Will we want more than one context instance per queue (with a specified device)? If so, the queue and device should be passed by parameter, and not created within the context.

Understood, thanks! But I think we only need one context instance per queue.

See my other comment.

src/webnn/native/dml/BackendDML.cpp

src/webnn/native/dml/ContextDML.cpp

bbernhar · 2022-08-18T17:49:59Z

src/webnn/native/dml/ContextDML.h

+
+namespace webnn::native::dml {
+
+    struct CommandRecorderDML {


Can we move this into .cpp? This class will become large-enough (for InitializeOperator, ExecuteOperator, etc).

You may mean InitializeGraph, ExecuteGraph, right? I think these will be in GraphDML.cpp.

GraphDML would not record the actual commands into the command-list. It just builds the operator (graph) for the CommandRecorder. The CommandRecorder itself records these commands via InitializeGraph, ExecuteGraph, etc. This maintains a clear separation of duties and avoids any state being shared between them (data-flow is one-way).

I highly encourage we all think about such things during the rewrite: clear boundaries, data-flow, etc.

Yes, the commandlist is only used after the graph building. I guess your suggestion for separation may be:
CommandRecorder -> IntializeGraph()/ExecuteGraph() ? That's why you suggested defining the CommandRecorder and ExecutionContext?

Can we move this into .cpp? This class will become large-enough (for InitializeOperator, ExecuteOperator, etc).

Will consider re-organizing codes when we add more functions.

src/webnn/native/dml/ContextDML.h

bbernhar · 2022-08-18T17:53:18Z

src/webnn/native/dml/ContextDML.h

+        static ContextBase* Create(ComPtr<IDXGIAdapter1> adapter, bool useDebugLayer);
+        ~Context() override = default;
+
+        CommandRecorderDML GetCommandRecorderDML() const {


Why do we need to get the CommandRecorder? The context can just execute it directly. Also, this returns a (copy) of the recorder, which is most definitely not what we want.

I plan to add InitializeGraph, ExecuteGraph in GraphDML.cpp which will use the commandRecord.

See my other comment.

bbernhar · 2022-08-18T17:57:06Z

src/webnn/native/dml/ContextDML.cpp

+        D3D12_COMMAND_QUEUE_DESC commandQueueDesc{};
+        commandQueueDesc.Type = D3D12_COMMAND_LIST_TYPE_DIRECT;
+        commandQueueDesc.Flags = D3D12_COMMAND_QUEUE_FLAG_NONE;
+        WEBNN_RETURN_IF_FAILED(mCommandRecorderDML.D3D12Device->CreateCommandQueue(


We can't have the context (itself) always create the [DML, D3D12] device and queue. Rather, we want the context to initialize and execute with any (device, queue) specified.

Also, the command allocator and list should be created by the recorder class you've defined. The recorder also accepts the (device, queue) as parameters to initialize it.

I don't understand, could you explain the purpose? @bbernhar
These D3D related members will only be created once when we create one context, we don't always create them. And I think one context owns one CommandRecorder I defined and the context will create multiple graphs, then graphs will do InitializeGraph and ExecuteGraph. Am I right? @fujunwei

The [DML, D3D12] device and queue does not have a 1:1 relationship with a context. You can have multiple contexts per {device, queue}. Chromium could for example, specify a {device, queue}, from GPUDevice, for a given context too. So, we cannot assume the context should always create a new one. In truth, the process only needs one queue (of each type).

In fact, if it does [create a new one], then we'll have to deal with complex cross-queue usage rules because resources shared between contexts wouldn't necessarily be compatible.

A CommandRecorderDML and the underlying IDMLCommandRecorder has 1:1 relationship, they should have the exact same lifetime.

Hopefully that makes more sense.

Make sense, we should enum all adapters (integrateGPU, discreteGPU, maybe VPU) at once in the GPU process for all contexts like WebGPU's Backend::DiscoverAdapters.

For each adapter in DML backend, there is a device(DML, D3D12) as member variable like WebGPU's mD3d12Device.

The device in DML backend has 1 queue and 1 commandRecorder to recoder commands for all contexts, the commands come from initialize and execute Graph, am i right?

@fujunwei Only the queue is sharable. Since no other recorder will be provided to this context to use, it doesn't need to outlive it. Every context has exactly one "live" recorder.

The whole purpose of CommandRecorderDML was to keep recorder-state together (list AND allocator). The command recorder can (and eventually will) enqueue multiple command-lists. Similarly, the context might also need to produce a entirely new command-list via new recorder.

Make sense, thanks for explain patiently.

bbernhar · 2022-08-22T21:13:55Z

src/webnn/native/dml/ContextDML.h

+
+    struct CommandRecorder {
+        ComPtr<IDMLDevice> device;
+        ComPtr<ID3D12Device> D3D12Device;


nit: please keep struct-field names consistent.

bbernhar · 2022-08-22T21:40:43Z

src/webnn/native/dml/ContextDML.cpp

+    };
+
+    // static
+    ContextBase* Context::Create(const ComPtr<IDMLDevice>& device,


Please do not pass a modifiable object like this. The point of passing ComPtr by value is to know that ownership could be shared and this (passing by ComPtr&) makes it ambiguous at the call-site. If you want to avoid the ref, wrap it in std::move instead. For example, Context::Create(std::move(comptr)) and then new Context(std::move(comptr)), ...)

OK, got it. I think maybe multiple graphs will share these Comptr, so I decide to copy.

mingmingtasd · 2022-08-23T03:14:10Z

PTAL, if there are no more significant comments, I intend to stop maintaining this PR and move to implement based on Mojo infrastrucure in otcshare/Chromium in this webnn_mojo branch to align with our team's main workflow now. Then you can go on reviewing codes there. Thanks! @bbernhar @fujunwei @miaobin

fujunwei · 2022-08-23T07:03:22Z

src/webnn/native/dml/BackendDML.cpp

+        }
+
+        // Create the D3D device.
+        if (FAILED(D3D12CreateDevice(adapter.Get(), D3D_FEATURE_LEVEL_11_0,


As we disscusion, the device and command queue can't be created when creating context, they should be created when connecting DML backend.

fujunwei · 2022-08-23T07:12:20Z

src/webnn/native/dml/ContextDML.h

+
+namespace webnn::native::dml {
+
+    struct CommandRecorder {


1, Move it to cpp files.
2, We should comment which variables are sharable between different context (Device, Queue)

fujunwei · 2022-08-23T07:13:57Z

src/webnn/native/dml/ContextDML.cpp

+        D3D12_COMMAND_QUEUE_DESC commandQueueDesc{};
+        commandQueueDesc.Type = D3D12_COMMAND_LIST_TYPE_DIRECT;
+        commandQueueDesc.Flags = D3D12_COMMAND_QUEUE_FLAG_NONE;
+        WEBNN_RETURN_IF_FAILED(mCommandRecorderDML.D3D12Device->CreateCommandQueue(


Make sense, thanks for explain patiently.

…ackend

mingmingtasd · 2022-08-25T08:49:43Z

As you suggested , we are migrating to the chromium's mojo based branch. Thanks! @bbernhar

mingmingtasd force-pushed the dml_rewrite branch 2 times, most recently from d6f6a3d to cdfe64e Compare August 3, 2022 05:35

bbernhar suggested changes Aug 3, 2022

View reviewed changes

mingmingtasd mentioned this pull request Aug 4, 2022

Delete the implementation of DML backend and disable CI #288

Merged

bbernhar reviewed Aug 4, 2022

View reviewed changes

src/webnn/native/dml/UtilsDML.h Outdated Show resolved Hide resolved

mingmingtasd force-pushed the dml_rewrite branch 2 times, most recently from 9697ccc to 19fef0d Compare August 8, 2022 07:51

mingmingtasd force-pushed the dml_rewrite branch from 19fef0d to eaf00a2 Compare August 9, 2022 05:07

mingmingtasd force-pushed the dml_rewrite branch from eaf00a2 to 6ab2258 Compare August 12, 2022 02:30

mingmingtasd mentioned this pull request Aug 15, 2022

Add CI for DML backend #292

Closed

mingmingtasd commented Aug 16, 2022

View reviewed changes

src/webnn/native/dml/dml_platform.h Show resolved Hide resolved

bbernhar suggested changes Aug 16, 2022

View reviewed changes

mingmingtasd force-pushed the dml_rewrite branch 3 times, most recently from 89f3982 to 1e937c9 Compare August 18, 2022 05:53

bbernhar reviewed Aug 18, 2022

View reviewed changes

src/webnn/native/dml/ContextDML.h Outdated Show resolved Hide resolved

bbernhar reviewed Aug 18, 2022

View reviewed changes

mingmingtasd force-pushed the dml_rewrite branch from 1e937c9 to 81c59b5 Compare August 22, 2022 03:26

bbernhar reviewed Aug 22, 2022

View reviewed changes

mingmingtasd force-pushed the dml_rewrite branch from 81c59b5 to 9bfd240 Compare August 23, 2022 03:03

fujunwei reviewed Aug 23, 2022

View reviewed changes

Create and initialize Device object: the first PR for rewriting DML b…

aab6d9b

…ackend

mingmingtasd force-pushed the dml_rewrite branch from 9bfd240 to aab6d9b Compare August 23, 2022 10:09

Create and initialize Device object: the first PR for rewriting DML backend #287

Are you sure you want to change the base?

Create and initialize Device object: the first PR for rewriting DML backend #287

Conversation

mingmingtasd commented Aug 3, 2022

bbernhar commented Aug 3, 2022 • edited Loading

bbernhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

mingmingtasd commented Aug 4, 2022 • edited Loading

fujunwei commented Aug 4, 2022

mingmingtasd commented Aug 4, 2022

bbernhar commented Aug 4, 2022

mingmingtasd commented Aug 8, 2022

miaobin commented Aug 9, 2022

bbernhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

bbernhar Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

mingmingtasd Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbernhar Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingmingtasd commented Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingmingtasd commented Aug 25, 2022

bbernhar commented Aug 3, 2022 •

edited

Loading

bbernhar Aug 5, 2022 •

edited

Loading

bbernhar Aug 19, 2022 •

edited

Loading

mingmingtasd commented Aug 4, 2022 •

edited

Loading

bbernhar Aug 19, 2022 •

edited

Loading

bbernhar Aug 18, 2022 •

edited

Loading

bbernhar Aug 19, 2022 •

edited

Loading

mingmingtasd Aug 22, 2022 •

edited

Loading

bbernhar Aug 18, 2022 •

edited

Loading

bbernhar Aug 18, 2022 •

edited

Loading

bbernhar Aug 19, 2022 •

edited

Loading

bbernhar Aug 22, 2022 •

edited

Loading

bbernhar Aug 22, 2022 •

edited

Loading

mingmingtasd commented Aug 23, 2022 •

edited

Loading