Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only output bo needs to be synced from device after result is available #849

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+8 −5
Open

Only output bo needs to be synced from device after result is available #849

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1228cf7
Split the big building script into multiple stages: configure/build/i…
dezhiAmd Sep 20, 2024
9003f1f
small change
dezhiAmd Oct 15, 2024
ae3024c
Merge branch 'main' into buildDebug
Oct 18, 2024
c47b1c6
Sync only ofm bo
Oct 18, 2024
1fd9113
Remove unrelated build scripts
Oct 18, 2024
336c485
Fix a typo (copy & paste error)
Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ artifacts/

# Local cache files
llvm-external-projects/iree-dialects/.cache
.build-cache

# pkgci artifacts
artifacts/
Expand Down
12 changes: 7 additions & 5 deletions runtime/src/iree-amd-aie/driver/xrt/direct_command_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,11 +333,10 @@ static iree_status_t iree_hal_xrt_direct_command_buffer_dispatch(
// Third argument is the number of LX6 instructions.
run.set_arg(arg_index++, kernel_params.num_instr);

xrt::bo ofm_bo;

// Copy descriptors from all sets to the end of the current segment for later
// access.
// TODO(jornt): hack to ensure that the output buffer is synced by syncing all
// buffers after the run.
std::vector<xrt::bo> bos;
// TODO(max): do we need multiple descriptor sets ever for AIE?
uint32_t set = 0;
IREE_RETURN_AND_END_ZONE_IF_ERROR(
Expand All @@ -348,8 +347,11 @@ static iree_status_t iree_hal_xrt_direct_command_buffer_dispatch(
xrt::bo(*command_buffer->descriptor_sets[set].bindings[j],
command_buffer->descriptor_sets[set].lengths[j],
command_buffer->descriptor_sets[set].offsets[j]);
bos.push_back(arg_buffer);
run.set_arg(arg_index + j, arg_buffer);
bool not_ofm = (bindings.values[j].buffer->memory_type & IREE_HAL_MEMORY_TYPE_HOST_VISIBLE) &&
(bindings.values[j].buffer->allowed_usage & IREE_HAL_MEMORY_TYPE_HOST_VISIBLE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not how input/output is determined - https://github.com/nod-ai/iree-amd-aie/blob/makslevental/xrt-lite/runtime/src/iree-amd-aie/driver/xrt/cts/executable_cache_test.mlir#L21-L23

      %0 = hal.interface.binding.subspan layout(#pipeline_layout) binding(0) alignment(64) offset(%c0) flags("ReadOnly|Indirect") : !flow.dispatch.tensor<readonly:tensor<32x32xf32>>
      %1 = hal.interface.binding.subspan layout(#pipeline_layout) binding(1) alignment(64) offset(%c0) flags("ReadOnly|Indirect") : !flow.dispatch.tensor<readonly:tensor<32x32xf32>>
      %2 = hal.interface.binding.subspan layout(#pipeline_layout) binding(2) alignment(64) offset(%c0) flags(Indirect) : !flow.dispatch.tensor<writeonly:tensor<32x32xf32>>

note !flow.dispatch.tensor<readonly:tensor<32x32xf32> vs !flow.dispatch.tensor<writeonly:tensor<32x32xf32>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, as it clearly states, IREE_HAL_MEMORY_TYPE_HOST_VISIBLE is a iree_hal_memory_type_bits_t flag, not an iree_hal_buffer_usage_bits_t flag. See https://github.com/iree-org/iree/blob/fbf677d2096aea4c544cd8d0558b49bfc729fd91/runtime/src/iree/hal/buffer.h

Copy link
Author

@dezhiAmd dezhiAmd Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What I see is all input bo are created from here

mappable_params.type |= IREE_HAL_MEMORY_TYPE_HOST_VISIBLE;
mappable_params.usage |= IREE_HAL_BUFFER_USAGE_MAPPING;

Copy link
Collaborator

@makslevental makslevental Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Repeating again: that is not the only place in HAL where bufferes are created.

Specifically those are the buffer_view APIs, which we do not use. We use the pure allocate_buffer APIs:

iree-amd-aie/runtime/src/iree-amd-aie/driver/xrt/direct_allocator.cc

Line 143 in fad9629

static iree_status_t iree_hal_xrt_allocator_allocate_buffer(

For example, xrt_lite_dispatch_test.log
For example, xrt_dispatch_test.log

Copy link
Author

Choose a reason for hiding this comment

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

Will this change affect xrt-lite? I thought this change only affect xrt

Copy link
Collaborator

@makslevental makslevental Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

image

Copy link
Author

Choose a reason for hiding this comment

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

Function iree_hal_buffer_view_generate_buffer_in_situ is at iree level. It will call xrt level function as you pointed out. I do not see a contradiction here.

Copy link
Collaborator

@makslevental makslevental Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Function iree_hal_buffer_view_generate_buffer_in_situ is at iree level. It will call xrt level function as you pointed out.

You're completely wrong. Feel free to run https://github.com/nod-ai/iree-amd-aie/blob/main/runtime/src/iree-amd-aie/driver/xrt/cts/matmul_dispatch_test.cc and see for yourself.

Copy link
Author

Choose a reason for hiding this comment

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

What I see is that the file you referenced has this line
#include "iree/hal/buffer_view_util.h"
That is aligned with my understanding.

Copy link
Collaborator

@makslevental makslevental Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

you're understanding is wrong: #852

feel free to consult any reference on how C++ headers work.

if (!not_ofm)
ofm_bo = arg_buffer;
}

run.start();
Expand All @@ -360,7 +362,7 @@ static iree_status_t iree_hal_xrt_direct_command_buffer_dispatch(
return iree_make_status(IREE_STATUS_UNKNOWN, e.what());
}

for (xrt::bo& bo : bos) bo.sync(XCL_BO_SYNC_BO_FROM_DEVICE);
ofm_bo.sync(XCL_BO_SYNC_BO_FROM_DEVICE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just because today we only have one output, doesn't mean tomorrow we will only have one output.

Copy link
Author

Choose a reason for hiding this comment

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

So far all NPU products only use one output buffer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This has absolutely nothing to do with NPU and everything to do with the model.

Copy link
Author

Choose a reason for hiding this comment

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

This has absolutely nothing to do with NPU and everything to do with the model.

Do we have a model running on NPU with multiple output bo?

Copy link
Contributor

Choose a reason for hiding this comment

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

This indeed doesn't have anything to do with NPU and whether we currently have a model like this or not. We shouldn't make the assumption that there is a single output buffer.


IREE_TRACE_ZONE_END(z0);
return iree_ok_status();
Expand Down
Loading