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

[SYCL][Doc] Move internal design docs #5556

Merged
merged 3 commits into from Feb 16, 2022

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Feb 11, 2022

Move all the internal design documents to a separate directory. This
declutters the "sycl/doc" directory, making it easier to find the
end-user documentation. It also moves several documents out of
"sycl/doc/extensions" which were not actually extension specifications.

We decided that the "sycl/doc/dev" directory should hold documents
related developers of DPC++ (not for users of DPC++), so rename it
accordingly.

Move all the internal design documents to a separate directory.  This
declutters the "sycl/doc" directory, making it easier to find the
end-user documentation.  It also moves several documents out of
"sycl/doc/extensions" which were not actually extension specifications.

We decided that the "sycl/doc/dev" directory should hold BKMs (best
known methods) for developers, so rename it accordingly.
@gmlueck gmlueck requested a review from a team February 11, 2022 22:37
@gmlueck gmlueck requested review from a team as code owners February 11, 2022 22:37
@Pennycook
Copy link
Contributor

I don't think the names "internal design" and "development BKMs" are consistent.

The design documents are only "internal" to the project, so I'm not sure what extra information "internal" is supposed to convey. The fact that the development BKMs aren't marked internal suggests that they're for "external" developers (end users?) but the ABI policy guide at least is targeted at developers who will be opening PRs.

I don't feel strongly about this, but suggest the following names instead:

  • design/ => design documentation
  • developer/ => developer documentation (removing BKM because its meaning isn't obvious)

Maybe everything else could go in:

  • user/ => user documentation

....but I appreciate that might break documentation builds and such.

@@ -1104,7 +1104,7 @@ Release notes for commit range c9d50752..5d7e0925
- The SYCL library doesn't guarantee stable API/ABI, so applications compiled
with older version of the SYCL library may not work with new one.
The workaround is to rebuild the application.
[ABI policy guide](doc/ABIPolicyGuide.md)
[ABI policy guide](doc/development-bkms/ABIPolicyGuide.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to development-policies or development-guidelines ?
BKMs are something one may ignore, that's not what ABI doc is.

Copy link
Contributor

Choose a reason for hiding this comment

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

developer documentation as suggested by John is also ok

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 opposed to changing the directory names. Does everyone like the following:

  • sycl/doc/extensions - Extension specifications
  • sycl/doc/design - Design documents
  • sycl/doc/developer - Developer information

Personally, I think it's better to keep end-user documentation directly in sycl/doc because it is more prominent. I also think there are more likely to be external links to those documents, so there is some value to keeping them where they are.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM as well.

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 would like to hear from @alexbatashev to see if he agrees. Updating all the links is kind of a pain, so I only want to do it once more. :-)

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 renamed the directories in b00236c. I also changed the titles in the Sphinx documentation in 429f2da

sycl/doc/conf.py Outdated
Comment on lines 53 to 57
# Sphinx complains about syntax errors in these files.
'internal-design/DeviceLibExtensions.rst',
'internal-design/SYCLPipesLoweringToSPIRV.rst',
'internal-design/fpga_io_pipes_design.rst',
'internal-design/Reduction_status.md'
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 we should fix syntax errors and include these documents. @gmlueck, could you file a ticker for that issue, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. I'll do this once we agree on the directory name.

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 filed an internal bug report.

Rename these two subdirectories based on review feedback.
@bader
Copy link
Contributor

bader commented Feb 15, 2022

@pvchupin
Copy link
Contributor

Ping @intel/llvm-reviewers-runtime for review

@pvchupin
Copy link
Contributor

Just noticed the article published today https://www.intel.com/content/www/us/en/developer/articles/technical/expanding-oneapi-support-for-languages-and-accelerators.html?cache=false#gs.p79jmz, which references https://github.com/intel/llvm/blob/sycl/sycl/doc/CompilerAndRuntimeDesign.md. This PR breaks the link. @pglowney, do you know if we can update the article?

It was adjusted to the new link.

@gmlueck
Copy link
Contributor Author

gmlueck commented Feb 16, 2022

It was adjusted to the new link.

@bader @pvchupin: It would be good to merge this soon. The end-user article noted above points to the new location of this design document, so the link will be broken until this PR is merged.

If we are only waiting for someone from @intel/llvm-reviewers-runtime, the changes in the runtime are extremely small. These two files just had a 1-line change to a comment:

  • sycl/source/detail/context_impl.hpp
  • sycl/source/detail/program_manager/program_manager.hpp

And this file had a few trivial changes to the pathname in an output string:

  • sycl/tools/abi_check.py

@bader
Copy link
Contributor

bader commented Feb 16, 2022

It was adjusted to the new link.

@bader @pvchupin: It would be good to merge this soon. The end-user article noted above points to the new location of this design document, so the link will be broken until this PR is merged.

It think it takes time to propagate the update to the web site. I still see the old link:
image

@gmlueck
Copy link
Contributor Author

gmlueck commented Feb 16, 2022

It think it takes time to propagate the update to the web site. I still see the old link:

I see the new link. You probably have the old version cached in your browser. Try reloading with CTRL-refresh.

@bader bader merged commit edbfc99 into intel:sycl Feb 16, 2022
@bader
Copy link
Contributor

bader commented Feb 16, 2022

@gmlueck gmlueck deleted the gmlueck/mv-internal-design branch February 16, 2022 14:29
smaslov-intel pushed a commit to smaslov-intel/llvm that referenced this pull request Feb 19, 2022
Move all the internal design documents to a separate directory.  This
declutters the "sycl/doc" directory, making it easier to find the
end-user documentation.  It also moves several documents out of
"sycl/doc/extensions" which were not actually extension specifications.

We decided that the "sycl/doc/dev" directory should hold BKMs (best
known methods) for developers, so rename it accordingly.
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Feb 23, 2022
…/llvm into refactor_existing_workflows

* 'refactor_existing_workflows' of github.com:alexbatashev/llvm: (2051 commits)
  [SYCL][L0] Honor property::queue::enable_profiling (intel#5543)
  [SYCL][CI] Enable sccache on Windows (intel#5589)
  [SYCL][Doc] Move internal design docs (intel#5556)
  [sycl-post-link] Initialize the integer Value variable (intel#5585)
  [CI] Fix nightly builds (intel#5584)
  [SYCL][L0] Fix use of copy-engines in L0 interop queue (intel#5579)
  Update OpenCL headers tag to dcd5bed (intel#5575)
  [SYCL] Fix warning for InOrderQueueSyncCheck unit test build (intel#5577)
  [SYCL][HIP] Remove arch requirement for running lit tests (intel#5253)
  [SYCL][L0] Fix timestamp calculation (in ns) (intel#5555)
  [SYCL] Fix sync of host task vs kernel for in-order queue (intel#5551)
  [sycl-post-link] Add a check for device globals with device_image_scope (intel#5517)
  [SYCL] Fix SYCL Kernel Body Check (intel#5546)
  [SYCL] Add support for SYCL 2020 in class group (intel#5447)
  Fix tests after 1c729d7 Use align attribute for kernel pointer arg alignment
  Fix tests after 18834dc Mark pointer-typed kernel arguments as ABI aligned
  [CI] Add experimental Windows build to GitHub Actions nightly (intel#5560)
  [sycl-post-link][NFC] Address clang-tidy concerns in the sycl-post-link (intel#5552)
  Fix lit test after changes in Clang
  Improve backward compatibility for DISubRange
  ...
@keryell
Copy link
Contributor

keryell commented Mar 5, 2022

Just looking at the (very good) documentation. Would it be possible to have only vector graphics format for images like SVG instead of JPG?
I have just noticed a renaming of sycl/doc/design/images/plugin-lifetime.jpg. Perhaps the source is lost...

@gmlueck
Copy link
Contributor Author

gmlueck commented Mar 7, 2022

Just looking at the (very good) documentation. Would it be possible to have only vector graphics format for images like SVG instead of JPG? I have just noticed a renaming of sycl/doc/design/images/plugin-lifetime.jpg. Perhaps the source is lost...

Yes, I think this is a good idea for new documents that we add. Currently, I think we have only these images that are not SVG:

  • sycl/doc/images/missing_sycl_dll.png
  • sycl/doc/design/Multi-source-compilation-flow.png
  • sycl/doc/design/images/plugin-lifetime.jpg

I don't know what happened to the source files for those images. If we need to edit them, we may need to recreate them as SVG.

@alexbatashev
Copy link
Contributor

sycl/doc/images/missing_sycl_dll.png

That's a screenshot. I can hardly imagine how we can covert it to SVG.

@keryell
Copy link
Contributor

keryell commented Mar 8, 2022

Ah yes, good point.
Otherwise use oneDNN running with DPC++ to do an image comprehension by recognizing the text and the font, then generate the SVG? :-)

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.

None yet

6 participants