Fix clc device selection on multi-device platform #788
Conversation
|
Hi, it would be good to get some context. Why would the devices have the same names? |
Maybe they shouldn't. The problem is that For more context, see the hal_refsi_tutorial example. It just keeps an instance of One extra weirdness is that the fact that the name is a I'd think that method should return by value and the returned object should use |
It also sets The assumption that the name uniquely identifies the device is kind of baked into our build system. We call |
57e3e55 to
3ebb4ed
Compare
This PR doesn't affect your build system. It cannot hurt, just help in case there's a name clash either by coincidence or by design. |
|
Note that |
|
And also note that it may be more convenient to select a device by index than by name regardless of potential name clashes. |
That is a strong argument in favour of your PR and I'm okay in principle with it on that basis, but note that the option there is not to deal with duplicate device names. I think we still have to consider duplicate device names an error. |
| @@ -219,7 +219,7 @@ class argument { | |||
| cargo::array_view<cargo::string_view> Choices; | |||
| }; | |||
| union { | |||
| bool *Bool; | |||
| bool *Bool = {}; | |||
There was a problem hiding this comment.
What is the reason for this change?
|
The compiler gives a warning if left uninitilized (as it happens with the
custom parser overload of the constructor).
This could all be much better if implemented with std::variant instead of
this ad-hoc semi tagged-union implementation, in my opinion. But that's a
different discussion.
…On Fri, Apr 25, 2025, 18:12 Harald van Dijk ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In modules/cargo/include/cargo/argument_parser.h
<#788 (comment)>
:
> @@ -219,7 +219,7 @@ class argument {
cargo::array_view<cargo::string_view> Choices;
};
union {
- bool *Bool;
+ bool *Bool = {};
What is the reason for this change?
—
Reply to this email directly, view it on GitHub
<#788 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5HBCMM4IM7PQCHWRYH76T23JNIFAVCNFSM6AAAAAB3ZDQS36VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOOJUGYZTMMRWGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***
com>
|
Ah, thanks, that makes sense. As you've seen, the idea is that |
|
That's a tagged-union. But in its basic form, it has problems. A plain union won't have an implicitly defined destructor if one of its members has a user-defined destructor. That's why simply adding CustomHandler to the union doesn't work. std::variant is in fact an advanced implementation of the tagged-union idea that correctly handles that sort of thing. That's why I suggested to use it instead of the plain union. That should be in a different PR, though. |
|
If now is not a good time to do that, then I don't know when :) Does hvdijk@cargo-argument-variant avoid that problem? If it does, I can put that up as a pull request first, or you can rebase your pull request on it and we can merge them together. I notice that |
|
That change is not related to this PR. You can do that after if you want.
The index being 1-based is because the list of available devices that gets
printed in case of error is 1-based.
…On Sat, Apr 26, 2025, 02:03 Harald van Dijk ***@***.***> wrote:
*hvdijk* left a comment (uxlfoundation/oneapi-construction-kit#788)
<#788 (comment)>
If now is not a good time to do that, then I don't know when :) Does
***@***.***
<hvdijk@cargo-argument-variant>
avoid that problem? If it does, I can put that up as a pull request first,
or you can rebase your pull request on it and we can merge them together.
I notice that muxc's --device-idx is zero-based, but your new clc's
--device-idx is one-based. Can you make sure that they work the same way?
—
Reply to this email directly, view it on GitHub
<#788 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5HBCP6WCHTGKG3GEB4MRD23LEODAVCNFSM6AAAAAB3ZDQS36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMZRGYZTMOJRGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
If a platform reports multiple devices, they will all have the same name and --device selection will be ambiguous. This adds a --device-idx cmd-line option to directly select by index.
3ebb4ed to
8af7a06
Compare
If a platform reports multiple devices, they will all have the same name and --device selection will be ambiguous.
This adds a --device-idx cmd-line option to directly select by index.