Skip to content

Draft of Metal driver api support#99

Draft
asindarov wants to merge 6 commits into
libmir:masterfrom
asindarov:asadbek-metal-driver-api-support
Draft

Draft of Metal driver api support#99
asindarov wants to merge 6 commits into
libmir:masterfrom
asindarov:asadbek-metal-driver-api-support

Conversation

@asindarov

Copy link
Copy Markdown

No description provided.

Comment thread source/dcompute/driver/metal/buffer.d Outdated
// Host memory associated with this buffer
T[] hostMemory;

this(T[] array)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this constructor should be a method of device

@asindarov asindarov Jun 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to follow the approach done in CUDA driver.

currently the following is how clients call create buffer and implicitly it copies the buffer to device memory as well:

	auto deviceX = Buffer!float(x);
	auto deviceY = Buffer!float(y);

should it be as follows:

	auto deviceX = Buffer!float(x);
	auto deviceY = Buffer!float(y);

	device.copy!float(deviceX);
	device.copy!float(deviceY);

or should I remove Buffer completely and move the logic there to Device ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create buffer and implicitly it copies the buffer to device memory as well

then it should be a method of device: auto buf = device.makeBuffer!float(hostMemory);

Comment thread source/dcompute/driver/metal/kernel.d Outdated
{
MTLComputePipelineState pipelineState;

this(MTLComputePipelineState ps)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

{
MTLLibrary metalLibrary;

Device device;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does the program contain a device?
The program represents the code.
It seems to me that you should split out the concept of a kernel from its binding to a pipeline state object.

import metal.argument;
import metal.types;

struct Queue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a pipeline state seems much more associated with a Queue.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes agreed, I moved it to Queue as it fits better there

// TODO(asadbek): explore options to make the use of async execution with events
this (Device _device /*bool async*/)
{
device = _device;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

device is unused outside of the constructor, why cache a reference to it in the Queue?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to encode it in other way, I used device within enqueue method's OpCall which makes it functor type of function, I can add a new parameter to get the device but I thought it would be nice to encode it as field instead of another parameter in the enqueue function

Comment thread source/dcompute/driver/metal/buffer.d Outdated
… Queue as it fits better there and store MTLFunction in kernel metadata instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants