[SYCL] Unify compile/run-time properties more #12245
Conversation
They had the unnecessary difference that for runtime the key was an alias. Advantage of unifying: - Usage is the unified (e.g. get_property always needs the key). - Simplifies implementation (future PR) which improves compilation speed
rolandschulz
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
rolandschulz
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
rolandschulz
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
rolandschulz
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
rolandschulz
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
rolandschulz
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| intel::experimental::cache_config_enum value; | ||
| }; | ||
| } // namespace ext::oneapi::experimental | ||
| namespace ext::intel::experimental { |
There was a problem hiding this comment.
You can see here that the definition of a typical runtime property expanded from 5 lines to about 12. However, the description of the PR says that a goal is to simplify the implementation. What is getting simplified? It seems like the definition of a runtime property is more complex, not less.
There was a problem hiding this comment.
What is getting simplified?
The main motivation is to simplify has_property and get_property. I'm worried that if/when properties are getting used a lot, that the current implementation will be bad for compilation speed. Currently we store the properties as a sorted list of values in the template argument of properties. And because not all have the same form (compile-time are of the form property_value<key, ...> and runtime are their own type) we have to iterate over the whole list every time.
With this change all property have the same form of property_value<key, ...>. This turns the sorted list into a sorted map. And with that efficient map lookup with mp_map_find (doc, impl) is possible.
If we don't like this change we could instead change the storage inside the template argument to a sorted set of runtime properties and a sorted map of compile-time properties (doesn't require any spec changes). Both would have fast lookup. We would just need to split the properties in the two group prior to sorting whenever we sort.
A 2nd motivation was to reduce where we need to distinguish between the two groups of properties in the implementation. With the alternative implementation this would become more not less.
For the spec we could allow both implementation strategies (runtime property values are of the form property_value<key, ...> or runtime property values/keys the same type). Then this becomes purely a quality of implementation question.
The property_value<key, ...> form allows a runtime property value to store some of its values as compile-time values. And those values can be queried at compile time. I'm not aware of use-cases for properties which have both runtime values (members) and also compile-time values (template args). So I don't know whether this is an advantage.
There was a problem hiding this comment.
I created a prototype of what I think a better implementation should roughly look like: https://godbolt.org/z/PvaWzTnnW. For the PRs I was trying to break the changes into incremental steps to help with review. But hopefully this shows what is possible after all changes. In my view it makes the implementation of the properties class much shorter and also easier to define both runtime and compile-time properties. This implementation currently isn't conformant, because the spec contains several implementation details which aren't valid for this strategy. To make it conformant we would need to remove (or change):
- Runtime properties key/value are the same type (this change)
- Remove key_t (Remove key_t #12246, not necessary and makes defining runtime properties simpler )
- Remove static from
get_property(simplifies implementation, we don't have any use case of non-device copyable properties) - Remove value_t (not necessary and simplifies defining properties a lot)
None of the changes are necessary. We could do none or only some and still simplify the implementation. It just that each of them make it simpler to either implement the properties class or add new properties.
I would be interested in any feedback regarding:
- Do we want the spec to mandate one implementation strategy or not?
- Which implementation strategy do we want to use?
- Do you prefer for review a set of incremental changes or one large PR?
There was a problem hiding this comment.
I prefer multiple incremental PRs, personally, and I don't think we should specify anything about implementation that we don't have to.
|
found better solution. not needed anymore |
They had the unnecessary difference that for runtime the key was an alias.
Advantage of unifying: