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.

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

Skeleton directory and build infrastructure for HIP bindings #7857

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

stsoe
Copy link
Collaborator

@stsoe stsoe commented Dec 19, 2023

Very preliminary infra for HIP bindings.
Build on linux with build.sh -hip [other options]

@stsoe stsoe requested a review from rozumx as a code owner December 19, 2023 01:38
Very preliminary infra for HIP bindings.
Build on linux with `build.sh -hip [other options]`

Signed-off-by: Soren Soe <[email protected]>
@gbuildx
Copy link
Collaborator

gbuildx commented Dec 19, 2023

Build Passed!

// Copyright (C) 2023 Advanced Micro Device, Inc. All rights reserved.
#include "device.h"

namespace xrthip_core {
Copy link
Member

Choose a reason for hiding this comment

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

Something more C++17 friendly and clearer like xrt::hip::core?

Copy link
Collaborator Author

@stsoe stsoe Dec 19, 2023

Choose a reason for hiding this comment

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

@keryell . I thought hard and long about this. I don't want xrt:: because that namespace is reserved for anything publicly visible from XRT, e.g. our public APIs. This is an internal namespace, the only thing that will eventually be exported are the APIs declarations, if any of these are C++ they will be in namespace xrt::hip. FYI, in xrt_coreutil we use namespace xrt_core for exactly the same reason. So, believe or not, like it or not, this is actually thought through, :-)

For reference, the internals of core XRT are in namespace xrt_core, so xrthip_core, kind of matches that; that was the thinking at least.

Copy link
Member

Choose a reason for hiding this comment

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

@stsoe So, believe or not, like it or not, this is actually thought through, :-)

Of course I think this is not some random writing. ;-) Thanks for the context.

Cf Doxygen \namespace to document a namespace. so this is not lost? :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@keryell Maybe I should relax the xrt:: constraint and clean up this nonsensical use of namespaces. Replace xrt_core with xrt::core, replace xrthip_core with xrt::hip::core. Continue to use xrt:: for publicly exported XRT native C++ APIs, use xrt::hip for publicly exported XRT C++ HIP APIs if any. At the same time I could use xrt::opencl::core for the opencl bindings which for historical reasons are in yet another obscure namespace xocl . Is that a more intuitive convention?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, maybe it should be xrt::core::hip, xrt::core::opencl which would make it much easier for the bindings implementation to reference xrt::core which will be used heavily?

@gbuildx
Copy link
Collaborator

gbuildx commented Dec 19, 2023

Build Passed!

@stsoe stsoe merged commit 9f739b4 into Xilinx:master Dec 19, 2023
18 checks passed
@stsoe stsoe deleted the hip branch December 19, 2023 20:45
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.

3 participants