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

[compute/cker] Introduce compile-time definitions file #14244

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomdol
Copy link
Contributor

@tomdol tomdol commented Oct 22, 2024

This commit introduces a modern C++ version of compile-time features detection utilities. The advantage of this approach over using #ifdef is mostly safety which C++ itself provides. The main idea for the usage of those checks is with if constexpr and in the context of SFINAE.

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak [email protected]

This commit introduces a modern C++ version of compile-time features detection utilities. The advantage of this approach over using #ifdef is mostly safety which C++ itself provides. The main idea for the usage of those checks is with `if constexpr` and in the context of SFINAE.

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak [email protected]
@tomdol
Copy link
Contributor Author

tomdol commented Oct 22, 2024

This PR is just for demonstration purposes. I'd like to ask for an opinion if a set of such utilities would make sense here.

In my opinion it could simplify some of the code, provide more safety than #ifdefs and would make the code a bit more readable in some cases. The file Common.h was modified just to demonstrate how those traits could be used in the context of a template function where only one branch of the constexpr if is compiled depending on the target architecture.

@glistening
Copy link
Contributor

glistening commented Oct 23, 2024

Thank you for your suggestion. It is interesting.

However, personally I don't feel benefit this modern c++ compile-time, it is verbose and not intuitive like the classic one. I can't find what safety error modern c++ style can detect more. Could you please let me know the example if any?

Also, I guess the modern style is not friendly to IDE syntax highlighting.

In addition, we need to keep one more type of template correspondences on top of the existing macro. It will add maintenance cost.

@tomdol
Copy link
Contributor Author

tomdol commented Oct 23, 2024

The non-preprocessor syntax is indeed more verbose but it is more c++ish way of checking things at compile time(just like it's done with https://en.cppreference.com/w/cpp/header/type_traits utilities). It's also more safe in the meaning that both the traits and their usages need to be a compilable, correct C++ while defines and preprocessor usage overall do not provide any safety checks. Thus anything can be used as a "value" of a define and as long as it can implicitly be cast to bool it will work whenever they are used in #ifdef or if statements.

The idea is related to this PR #14238 and to the branching in the ops implementations. Right now the runtime code is intermixed with preprocessor statements switching blocks of code on and off depending on the target architecture. I personally think it would be better to have a clear separation of paths so either separate functions containing target-specific intrinsics or separate if constexpr branches which would compile separately depending on the arch.

This is still just a loose suggestion though and an ask for opinion if this kind of changes would be acceptable in the cker module.

Lastly regarding the syntax highlighting I actually think the if constexpr approach is better because it doesn't make any part of the code "grayed out". Here's what I mean:
image

@glistening
Copy link
Contributor

Thank you for answer. I would like to hear other's opinions also.

In addition, for IDE, I mean that I can turn on/off the code blocks by defining some macro definition. But I am not sure it is possible in modern c++ template approach. It is the reason it may not be IDE-friendly.

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.

2 participants