[SYCL][Docs] Add proposed event mode extension

[steffenlarsen]

This commit adds the sycl_ext_intel_event_mode extension as proposed. To support this extension, the submission functions in the sycl_ext_oneapi_enqueue_functions extension are given property arguments without any current consumers.

[Pennycook]

I spotted a typo, but otherwise LGTM.

[gmlueck]

I think this should be a runtime property that takes a bool parameter. This will allow usage like:

bool low_power = /* check some option */;
oneapiex::properties props{intelex::low_power_event{low_power}});

Code like this is difficult with the API as you have it now because the existence of the low_power_event property affects the type of the props variable. As a result, you can not do this:

bool low_power = /* check some option */;
oneapiex::properties props;
if (low_power)
  /* no way to add 'low_power_event' property now */

If you make it a runtime property, the property's default constructor can set the property's value to true.

Alternatively, the property could take an enum instead of a bool. I think someone suggested in one of our meetings that there could be other types of low-power events in the future? I can't remember whether we decided that was realistic, though.

[steffenlarsen]

That's a good point. Would it make sense to change the property to an event_mode property and have the value as:

enum event_mode_enum {
  default_mode,
  low_power_mode
};

If we think future modes might overlap, we can also do it as a bitmap.

[Pennycook]

Am I right in thinking that we can define it as an enum now, and convert it to a bitmask later just by defining the relevant operators?

Either way, I like this direction. I'd be tempted to change the names, though, to avoid some redundancy:

enum class event_mode {
  none, // we can't use"default" because it's a keyword
  low_power,
};

[steffenlarsen]

Am I right in thinking that we can define it as an enum now, and convert it to a bitmask later just by defining the relevant operators?

Absolutely. As long as it doesn't cross the library boundary, we can change it later if we want.

[gmlueck]

I think the enum direction makes sense, but I'm not sure what names you are proposing. It seems like the name event_mode has been proposed as both the name of the property and the name of the enumeration. Can you write out an example usage to illustrate the naming that you propose?

[steffenlarsen]

I'm definitely not married to the name of the enum, but I just took inspiration from the naming of the compile-time property enums.

Adjusting the example in the text, the property would be used as

oneapiex::properties Props{intelex::event_mode{intelex::event_mode_enum::low_power}};

Note, I agree with @Pennycook in that it should be an enum class, mainly so it doesn't bleed its contents.

[gmlueck]

@Pennycook: Do you like this naming convention where the enum name is plural and the property name is singular?

The other alternative we discussed is to name the property event_mode and name the enum event_mode_enum.

[Pennycook]

A quick search of the ISO C++ specification suggests that their convention is to use a plural when the enum is a bitmask (e.g., std::filesystem::copy_options) but a singular when it is not (e.g., std::filesystem::file_type).

If we want to keep the plural here, then I think we should define this as a bitmask. But that also suggests that the property name should also be a plural. Didn't we recently discuss reintroducing a nested namespace for properties? That would allow sycl::event_modes to be a bitmask of event modes, and sycl::property::event_modes to be a property for setting an event's modes.

I don't really like the idea of encoding _enum in a name, because of the redundancy there.

[gmlueck]

I don't think we should make this a bitmask.

We have already used the _enum suffix for some other properties, so we have a precedent.

If we did add the namespace, why wouldn't we put the property and the enum in the same namespace? The enum is only used in conjunction with the property.

[gmlueck]

@steffenlarsen, I saw that you re-requested my review. I think the naming issue in this thread is not yet resolved.

If we can't think of a better solution, I suggest we go with the name event_mode_enum, which is consistent with existing properties that take an enum.

[steffenlarsen]

Ah, apologies. I glanced over this one. event_mode_enum is fine with me. 👍