uxlfoundation / oneAPI-spec

oneAPI Specification source files
https://spec.oneapi.com
Other
185 stars 107 forks source link

[oneDPL] Both `is_execution_policy` and `is_execution_policy_v` should be in `oneapi::dpl` namespace #558

Open rarutyun opened 1 month ago

rarutyun commented 1 month ago

Currently both utilities are in oneapi::dpl::execution namespace. However, there is misalignment with C++ standard where both is_execution_policy and is_execution_policy_v are in std namespace, not std::execution.

I believe we should fix that misalignment to meet user expectations.

Proposed resolution:

Note: Having is_execution_policy and is_execution_policy_v available in both oneapi::dpl and oneapi::dpl::execution namespaces is implementable.

akukanov commented 3 weeks ago

Would it be enough to add name aliases into oneapi::dpl with using declarations, or is it better to duplicate (perhaps with some note that an implementation based on aliases is conformant)?

Tagging @danhoeflinger

danhoeflinger commented 3 weeks ago

I think this is a good idea, I've wondered why they are placed there currently.

From the spec perspective, is the note about aliases necessary, or just as a clue for future implementers that this is the suggested way to implement it?

It looks like https://github.com/oneapi-src/oneDPL/pull/1457 does half of this in the implementation. It probably makes sense to do these at the same time. Would we want to do this in a standalone PR, or keep it in the ranges MVP?

rarutyun commented 3 weeks ago

Would it be enough to add name aliases into oneapi::dpl with using declarations, or is it better to duplicate (perhaps with some note that an implementation based on aliases is conformant)?

I would definitely go with aliases because duplicating types leads to other problems like metaprogramming, ADL, etc. In other words, compile-time stuff.

From the spec perspective, is the note about aliases necessary, or just as a clue for future implementers that this is the suggested way to implement it?

If I understand it correctly, with a clue of "suggested way to implement it" we are creating non-portable code for between Intel oneDPL and future implementations. I think the aliases are good way to specify that change. The only thing to consider is whether we guarantee something "Argument-dependent lookup"-wise. It might affect which namespace we define "type" in and which namespace we define "alias" in, for future implementations.

It looks like https://github.com/oneapi-src/oneDPL/pull/1457 does half of this in the implementation. It probably makes sense to do these at the same time. Would we want to do this in a standalone PR, or keep it in the ranges MVP?

I think it doesn't really matter. We can go with a separate PR or having everything in ranges MVP. I have a slight preference for a separate PR only because it's easier to link it to this very issue. It would be very clear that we resolve the issue with a specific and small PR rather than looking for necessary changes through the big modifications by ranges MVP. What I strongly agree with is that we need to make those changes in one PR (just pick some). I definitely don't want the half of the changes being implemented "here" and another half - "there"

akukanov commented 3 weeks ago

We cannot change the spec and the implementation in a single PR. Here I would like to discuss the spec.

The idea is that we want the is_execution_policy trait to be available in both oneapi::dpl::execution (to avoid breaking changes) and in oneapi::dpl (to be more consistent with the standard). We can do it with two distinct classes or with an alias. The problem with the alias is that it prescribes the implementation in a very specific way; and moreover, for the sake of minimizing changes I would prefer to keep the stuff in oneapi::dpl::execution as it is, which means that aliases are added into oneapi::dpl, though the only reason for this is a past mistake. I can live with it. however, I would like to consider such a variation of the spec that allows more implementation freedom by not defining strictly where the class is and where the alias is (or if there even is one).

I do not care much whether the trait classes are the same (i.e. one is an alias) or different, for as long as is_execution_policy<T>::value is the same for any T - because that's how the trait is supposed to be used, in particular in metaprogramming. Even if ADL could go "wrong", I am not much concerned. Am I wrong?

Finally, as it comes to is_execution_policy_v variable, I do not see any harm at all with "duplicating" it; do I miss something though?

rarutyun commented 3 weeks ago

We cannot change the spec and the implementation in a single PR. Here I would like to discuss the spec.

That's not what I was talking about. I suggested to have both is_execution_policy and is_execution_policy_v in one PR. In ranges MVP only is_execution_policy_v is touched as far as I can tell.

We can do it with two distinct classes or with an alias

I totally understand that.

for the sake of minimizing changes I would prefer to keep the stuff in oneapi::dpl::execution as it is, which means that aliases are added into oneapi::dpl

Sure. I would prefer it the other way around in general, and moreover if not that mistake I would prefer to have traits in oneapi::dpl namespace only. But I totally understand your preference with a given situation and I am full of support.

I do not care much whether the trait classes are the same (i.e. one is an alias) or different, for as long as is_execution_policy::value is the same for any T - because that's how the trait is supposed to be used, in particular in metaprogramming. Even if ADL could go "wrong", I am not much concerned. Am I wrong?

Perhaps, you are not. For sure, for T the trait is_execution_policy<T>::value should return the same result, no matter if it one trait and alias or if those are two distinct structs in different namespaces. I am talking about more hypothetical scenario when is_execution_policy struct itself is used for metaprogramming. Maybe, this scenario in not realistic and we should not case about them. I guess for ADL it's the same when is_execution_policy struct or object of such a type is an argument.

Finally, as it comes to is_execution_policy_v variable, I do not see any harm at all with "duplicating" it; do I miss something though?

From the top of my head you are right. Only the duplicating of "state".