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

[RFC][POC] Suggest re-design of compile-time properties extension #15752

Draft
wants to merge 13 commits into
base: sycl
Choose a base branch
from

Conversation

aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Oct 18, 2024

This is based on @rolandschulz 's #13776, with lots of help from both @rolandschulz and @tahonermann in offline chats.

This is a proof of concept on how the extension can be changed. The purpose of this PR is to start discussion on

  1. If the proposed redesign is acceptable/desirable
  2. If so, how should we implement it? Should we have a single PR that updates the extension, implementation and uses all at once? If yes, can we do that outside of the major release?

I plan on drafting the changes to the extension later on once the implementation prototype is closer to finalization.

As of now this PR adds new implementation and sycl::ext::oneapi::experimental::new_properties namespace and switches group_load_store extension to use that instead of old properties. I'll try to port something using runtime property later and will update the PR after that.

sycl/include/sycl/ext/oneapi/properties/properties.hpp Outdated Show resolved Hide resolved
sycl/test/extensions/properties/new_properties.cpp Outdated Show resolved Hide resolved
return true;
}
}(),
"Properties must be sorted and non-repeating!");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here we want to dragonize separately if the repeating is because there are conflicting sort_keys (sort_key is same but property_ty isn't).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also does this assert actual give a reasonable diagnostic in case of repeating properties? I had to add a special base case

// base case is only for invalid cases to suppress compiler errors about base
to get only the static assert. Otherwise the compiler would complain about duplicate base types and wouldn't even get to the assert.

BTW: We'll need unit tests for the expected errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with negative unit test. Not sure about repeating sort_key. I assume you're talking about the case when two distinct unrelated properties happened to have the same sort key by mistake. However, that is undistinguishable from a valid scenario when compile-time properties that carry extra information other than their mere presence do share the sort_key. For example,

enum class data_placement { blocked, striped };
template <data_placement placement>
struct data_placement_property
    : detail::property_base<data_placement_property<placement>,
                            struct data_placement_property_key> {
  static constexpr mock_property_sort_key_t sort_key = 3;
  static constexpr bool is_blocked() {
    return placement == data_placement::blocked;
  }
};

We don't want to allow properties{data_placement_property<blocked>{}, data_placement_property<striped>{}} and we catch that because they have the same sort key, but the fact that they do have it is by design and is intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add a special base case

Thanks! I wouldn't have guessed how to make that work on my own...

Copy link
Contributor

Choose a reason for hiding this comment

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

We can distinguish properties{data_placement_property<blocked>{}, data_placement_property<striped>{}} from properties{prop_a{}, prop_b{}} where propb_a and prob_b have the same sort-key by mistake. While in both cases the property type is different and the sort key is the same, the first case has the same property key type and the 2nd has two different property key types. This difference allows us to issue different diagnostic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think latest revision addresses this thread, please "resolve conversation" if you agree.

sycl/include/sycl/ext/oneapi/properties/properties.hpp Outdated Show resolved Hide resolved
int x = 42;
properties pl{ct_prop<42>{}, rt_prop{x}};
constexpr auto p = pl.get_property<struct ct_prop_key>();
static_assert(std::is_same_v<decltype(p), const ct_prop<42>>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if const ct_prop<42> instead of prop<42> is an issue, and if it is then I don't know how to address it.

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