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

More documentation improvements #1835

Merged
merged 17 commits into from
Sep 13, 2024
Merged

Conversation

akukanov
Copy link
Contributor

@akukanov akukanov commented Sep 10, 2024

This patch proposes

  • A new paragraph in "Introduction" that explains interactions with -fscyl-pstl-offload;
  • A reworked "known issue" note associated with PSTL offload;
  • Using links to the "current" versions of other oneAPI documents instead of some specific versions;
  • [WIP, not yet in the draft] Clarification of dependencies between policy types and passing data to algorithms
  • A number of modifications related to execution policies, in particular:
    • The terms "standard aligned" aka "host" execution policies for par_unseq, par etc.;
    • Consistent use of "SIMD execution" when referring to unsequenced policies
    • Extended and improved desccription

as well as formatting and other improvements.

Copy link
Contributor

@dcbenito dcbenito left a comment

Choose a reason for hiding this comment

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

Small changes, but otherwise LGTM.

documentation/library_guide/introduction.rst Outdated Show resolved Hide resolved
@akukanov akukanov marked this pull request as ready for review September 12, 2024 11:51
@akukanov akukanov requested a review from dcbenito September 12, 2024 11:51
@akukanov akukanov force-pushed the dev/doc-policy-improvements-akukanov branch from d40104a to 9457aa0 Compare September 12, 2024 15:49
dcbenito
dcbenito previously approved these changes Sep 12, 2024
Copy link
Contributor

@dcbenito dcbenito left a comment

Choose a reason for hiding this comment

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

I have two minor changes, otherwise LGTM.

Comment on lines +7 to +8
also referred to as *standard aligned* or *host execution policies*, and the *device execution policies*
to run data parallel computations on heterogeneous systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be more clear if we make this into two sentences. Change the comma to a period then start the next sentence as "|onedpl_short| also provides the device execution policies ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will think of it - but perhaps in another patch, unless bigger changes are requested for this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

not required, the rest LGTM. About to approve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed "and ..." to "as well as device execution policies", but otherwise left it as is. I do not want to split, as the "According to the spec" part applies to both policy kinds.

Copy link
Contributor

@timmiesmith timmiesmith left a comment

Choose a reason for hiding this comment

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

one small suggestion, but it looks good to me.

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM

in the ``oneapi/dpl/execution`` header. The policies have the following meaning:

====================== =====================================================
Policy Value or Type Description
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy Sep 13, 2024

Choose a reason for hiding this comment

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

Probably, does it make sense to "split" (to have two tables) for the host policies
(policy values only) and for device policy types and policy values?
Otherwise, the column "Policy Value or Type" might confuse a user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
The description indicates what is type and what is a value. I can also add "type" to the types :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried a few variations, and ended up with changing the column heading to "Policy Name / Type".

icpx -fsycl -fsycl-pstl-offload=gpu program.cpp -o program

This option redirects C++ parallel algorithms invoked with the ``std::execution::par_unseq`` policy
to |onedpl_short| algorithms. It does not change the behavior of the |onedpl_short| execution policies and algorithms
Copy link
Contributor

Choose a reason for hiding this comment

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

While I understand (after several reading) that onedpl_short is related to both execution policies and algorithms, perhaps adding it explicitly for the second time helps make this sentence clearer Maybe adding "themselves" also could help. I mean something like:

Suggested change
to |onedpl_short| algorithms. It does not change the behavior of the |onedpl_short| execution policies and algorithms
to |onedpl_short| algorithms. It does not change the behavior of the |onedpl_short| execution policies and |onedpl_short| algorithms themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"... that are directly used in the code" below should convey similar meaning. Anyway, I will think how to make it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved by swapping the words: "It does not change the behavior of the |onedpl_short| algorithms and
execution policies that are directly used in the code."

namespace, to a parallel algorithm.
#. Use the C++ standard execution policies:
#. Pass a |onedpl_short| execution policy object as the first argument to a parallel algorithm
to specify the desired execution behavior.
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy Sep 13, 2024

Choose a reason for hiding this comment

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

"desired execution behavior" - I remembered that a host execution policy doesn't guarantee execution behavior, it may have the desired behavior, it is just a hint.
A device policy provides the desired (device execution) execution behavior.
Probably this difference should be explained in the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will think of it.

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 rephrased it as "to indicate the desired behavior" to sound a bit less formal. Anyway, the sentence describes a user action, and the behavior is desired, not guaranteed. Also I added "possible" to execution descriptions for unseq and par.

@akukanov
Copy link
Contributor Author

The new feedback will be addressed in another patch.

@akukanov akukanov merged commit a8f47ea into main Sep 13, 2024
22 checks passed
@akukanov akukanov deleted the dev/doc-policy-improvements-akukanov branch September 13, 2024 21:13
mmichel11 pushed a commit that referenced this pull request Sep 25, 2024
* Align terminology and improve wording related to execution policies
* Add information about -fsycl-pstl-offload and improve the known issue note
* Add the limitation on the use of pointer-to-member objects
* Clarify the need for policies and data storage to match
* Formatting and stylistic improvements
timmiesmith pushed a commit that referenced this pull request Nov 15, 2024
* Align terminology and improve wording related to execution policies
* Add information about -fsycl-pstl-offload and improve the known issue note
* Add the limitation on the use of pointer-to-member objects
* Clarify the need for policies and data storage to match
* Formatting and stylistic improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants