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

[0027] Shader Execution Reordering: rename "ReorderThread" to "MaybeReorderThread" #378

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

damyanp
Copy link
Member

@damyanp damyanp commented Jan 31, 2025

As noted by @Jasper-Bekkers in this comment, "MaybeReorderThread" may a better name to make it clear that some implementations may not actually reorder threads at this point.

@amarpMSFT
Copy link
Collaborator

I'd suggest TryReorderThread(), the "Try" has some precedence in other APIs IIRC. Maybe maybe does as well though :).

@damyanp
Copy link
Member Author

damyanp commented Jan 31, 2025

I'd suggest TryReorderThread(), the "Try" has some precedence in other APIs IIRC. Maybe maybe does as well though :).

So I did have a version that used Try, and I felt icky doing that one because I strongly expect a function called TryXXX to return a boolean indicating whether it did or did not do what was asked.

@Jasper-Bekkers
Copy link

I'd suggest TryReorderThread(), the "Try" has some precedence in other APIs IIRC. Maybe maybe does as well though :).

The suggestion came from https://doc.rust-lang.org/beta/std/mem/union.MaybeUninit.html - it other places Try also got suggested but it wouldn't have my preference since it would imply a slightly different behavior: "try to do this operation and return success/failure", rather then "I'll do this, or not".

@rasmusnv
Copy link
Contributor

rasmusnv commented Feb 3, 2025

Could we instead add a feature flag that indicates whether ReorderThread is expected to be honored or be a no-op on a particular device? Of course, a particular invocation of ReorderThread could be ignored, e.g., at the tail end of a dispatch as work is running out, but I'm not sure that is actionable information.

@damyanp
Copy link
Member Author

damyanp commented Feb 3, 2025

Could we instead add a feature flag that indicates whether ReorderThread is expected to be honored or be a no-op on a particular device? Of course, a particular invocation of ReorderThread could be ignored, e.g., at the tail end of a dispatch as work is running out, but I'm not sure that is actionable information.

I think we should consider adding the feature flag regardless, but I don't think that changes the feedback on the name. When authoring / reading shaders, you don't necessarily know what device it will run on so the "Maybe" clue I think still has value.

@llvm-beanz
Copy link
Collaborator

My $0.02: I like Maybe rather than Try, for much the reason @damyanp listed above. Try implies that you want it to try to do something and you want some notice if it happened. If we were to name the API Try I would want it to also return a bool signaling if thread re-ordering occurred.

Both "maybe" and "try" have usage in the C++ standard. The general convention is "try" returns something indicating if it was successful (https://en.cppreference.com/w/cpp/thread/unique_lock/try_lock, https://en.cppreference.com/w/cpp/container/map/try_emplace, https://en.cppreference.com/w/cpp/thread/latch/try_wait, etc...). The only usage I see for "maybe" is to have the compiler ignore something if it occurs (https://en.cppreference.com/w/cpp/language/attributes/maybe_unused).

MaybeReorderThreads I think clearly conveys that the call may or may not reorder threads, and we're not going to tell the user if it did.

@damyanp damyanp merged commit 2a084df into microsoft:main Feb 20, 2025
1 check passed
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.

6 participants