VITIS-15769 Deprecate legacy image management APIs in favor of xrt::…#9651
VITIS-15769 Deprecate legacy image management APIs in favor of xrt::…#9651chvamshi-xilinx merged 6 commits intoXilinx:masterfrom
Conversation
…hw_context-based APIs Signed-off-by: karthik dmg <karthdmg@xcokarthdmg50x.amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
stsoe
left a comment
There was a problem hiding this comment.
Can our internal code and tests be fixed? If not, then I don't think the APIs can be deprecated.
| * | ||
| * This constructor initializes a buffer object with the specified device, xclbin UUID, and string identifier. This throws an exception if no GMIO/External buffer exists with given name | ||
| */ | ||
| [[deprecated("deprecated, please use corresponding hw_context function instead")]] |
There was a problem hiding this comment.
In my experience, you need to provide the API to use and not leave the user guessing.
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wdeprecated-declarations" | ||
| auto sender_receiver_k1 = | ||
| xrt::kernel(device, uuid, "sender_receiver:{sender_receiver_1}"); | ||
| auto controller_k1 = | ||
| xrt::kernel(device, uuid, "pl_controller_kernel:{controller_1}"); | ||
| #pragma GCC diagnostic pop |
There was a problem hiding this comment.
What deprecated functions does this hide? Is it not possible to fix test tests?
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wdeprecated-declarations" | ||
| if (retVal == EOPNOTSUPP) { | ||
| krnl_name = "slavebridge"; | ||
| xclbin_uuid = device.load_xclbin(old_binary_file.string()); | ||
| } else { | ||
| xclbin_uuid = device.load_xclbin(b_file); | ||
| } | ||
| #pragma GCC diagnostic pop |
There was a problem hiding this comment.
If this cannot be fixed, then the APIs cannot be deprecated.
src/runtime_src/xrt/device/hal2.cpp
Outdated
| #if defined(__GNUC__) | ||
| # pragma GCC diagnostic ignored "-Wdeprecated-declarations" | ||
| #endif |
There was a problem hiding this comment.
What APIs are used here that are deprecated. If the code cannot be fixed, then APIs cannot be deprecated.
Signed-off-by: Karthik Dmg <karthdmg@xcokarthdmg50x.amd.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: karthik dmg <karthdmg@xcokarthdmg50x.amd.com>
| xrt::hw_context hw_ctx(device, uuid); | ||
|
|
||
| std::vector<xrt::kernel> krnls = create_kernel_objects(device, xclbin_uuid, num_kernel); | ||
| std::vector<xrt::kernel> krnls = create_kernel_objects(hw_ctx, num_kernel); |
There was a problem hiding this comment.
warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
std::vector<xrt::kernel> krnls = create_kernel_objects(hw_ctx, num_kernel);
^| try { | ||
| if (num_kernel_ddr) { | ||
| auto throughputs = test_bandwidth_ddr(device, krnls, num_kernel_ddr); | ||
| auto throughputs = test_bandwidth_ddr(hw_ctx, krnls, num_kernel_ddr); |
There was a problem hiding this comment.
warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
auto throughputs = test_bandwidth_ddr(hw_ctx, krnls, num_kernel_ddr);
^| } | ||
| if (chk_hbm_mem) { | ||
| double max_throughput = test_bandwidth_hbm(device, krnls, num_kernel); | ||
| double max_throughput = test_bandwidth_hbm(hw_ctx, krnls, num_kernel); |
There was a problem hiding this comment.
warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
double max_throughput = test_bandwidth_hbm(hw_ctx, krnls, num_kernel);
^| auto bandwidth_kernel = xrt::kernel(hw_ctx, "bandwidth_kernel"); | ||
|
|
||
| auto max_throughput_bo = xrt::bo(device, 4096, bandwidth_kernel.group_id(1)); | ||
| auto max_throughput_bo = xrt::bo(hw_ctx, 4096, bandwidth_kernel.group_id(1)); |
There was a problem hiding this comment.
warning: 4096 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
auto max_throughput_bo = xrt::bo(hw_ctx, 4096, bandwidth_kernel.group_id(1));
^Signed-off-by: karthik dmg <karthdmg@xcokarthdmg50x.amd.com>
Signed-off-by: karthik dmg <karthdmg@xcokarthdmg50x.amd.com>
stsoe
left a comment
There was a problem hiding this comment.
Thank you. Good to see that we at least can fix our own code to not use the deprecated APIs. One small suggestion about header include.
| #include "xrt/xrt_device.h" | ||
| #include "xrt/xrt_bo.h" | ||
| #include "xrt/xrt_hw_context.h" | ||
| #include "xrt/experimental/xrt_xclbin.h" |
There was a problem hiding this comment.
It doesn't look like this header file requires xrt_xclbin.h, include-what-you-use, probably move to .cpp?
This PR address changes to the OpenCL XRT flow. In hwctx flow, we create hw contexts from xclbins on demand instead of explicitly loading the xclbins. The created hwctxs are cached in hal device and accessors are provided so that xclbin references can be converted into the hwctx that has loaded the xclbin. Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Signed-off-by: karthik dmg <karthdmg@xcokarthdmg50x.amd.com>
Problem solved by the commit
Added warning for this release to deprecate any XRT requirements associated with v++ --package.generateAiePdi=1 that are not supported through xrt::hw_context.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
Added warning for this release to deprecate any XRT requirements associated with v++ --package.generateAiePdi=1 that are not supported through xrt::hw_context.
How problem was solved, alternative solutions (if any) and why they were rejected
Added warning for this release to deprecate any XRT requirements associated with v++ --package.generateAiePdi=1 that are not supported through xrt::hw_context.
Risks (if any) associated the changes in the commit
None
What has been tested and how, request additional testing if necessary
Ran XRT validate test suite to verify warning logged while load xclbins.
Ran application that call create kernels, create ext. BOs, graph creation APIs
Documentation impact (if any)
Attached .xls sheet in jira that has a list of APIs that log deprication warning