-
Notifications
You must be signed in to change notification settings - Fork 755
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] Initial spec conversion to Sphinx #16540
Open
gmlueck
wants to merge
3
commits into
intel:sycl
Choose a base branch
from
gmlueck:gmlueck/sphinx-doc
base: sycl
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this option?
LLVM and Clang documentation uses
:doc:
syntax and I guess all users rely on HTML documentation for links to work.You suggest we use different links style for writing SYCL documentation to enable cross-document links for GitHub rendered documents. I'm not sure how we can catch the use of wrong notation.
Can we ignore the GitHub rendering issue and follow LLVM/Clang link notation (i.e. use https://intel.github.io/llvm/ rendered documentation)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is extremely common for us to point people to the GitHub documentation for extensions. We do this all the time in email and internal trackers too. Does https://intel.github.io/llvm/ get updated automatically to reflect the top of the "sycl" (main) branch in the repo? If so, I guess we could get in the habit of pointing people there instead.
One problem could occur if we need to point people to the documentation in a particular branch of the repo. Maybe this is uncommon enough that it doesn't matter, though.
On the other hand, using the alternate link syntax seems easy to do. Is it really so offensive? It would be nice to be able to continue pointing people to the documentation in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We update it every night and each time anything changes in
clang/docs/
orsycl/doc/
directories. Exact rules:llvm/.github/workflows/sycl-docs.yml
Lines 4 to 19 in c064279
I don't know if using alternative style for SYCL documentation references is a real issue for the project, but is using Asciidoc format is a real blocker for upstreaming SYCL?
MLIR project managed to get accepted although they use Markdown format.
To be fair, LLVM and Clang sub-project also use Markdown alongside reStructuredText format, but MLIR rendered version has completely different look than LLVM/Clang documentation as they use Hugo framework to generate website instead of Sphinx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. When I talked to @AaronBallman, he seemed to think that converting to RST/Sphinx was important. I'm not sure if it's an absolute requirement, though.
I'd be OK with any of these options, but they all have some sort of down-side:
Option 1
Option 2
Option 3
Option 4
To give some idea of the magnitude of the work, there are over 100 extensions ("supported", "experimental", and "proposed" combined), and their specifications total over 40K lines of text.
@bader, have you given any thought to what we will do with all the other SYCL documents in "sycl/doc" that are not extension specifications? Will we convert this to RST/Sphinx? Will the generated HTML be hosted someplace, or will the files just live in the repo?
EDIT: The total count of extension specifications includes "supported", "experimental", and "proposed". A previous version of this comment omitted the word "supported" from that list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't plan any changes in
sycl/doc
for the upstreaming. From day one, we use Markdown format for SYCL documentation to be upstream-ready. LLVM sub-projects use Markdown in addition to RST (even more extensively after moving to GitHub to be more friendly for first-time/irregular contributors).I expect documentation for SYCL library will be in HTML format and will be hosted as at llvm.org as other sub-projects. See https://releases.llvm.org/ for examples. To make this happen we must upstream the minimal version of the working implementation fist.
Tagging @sergey-semenov, in case he has other plans. According to my understanding @sergey-semenov is working on upstreaming everything in
sycl
directory.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, all of the extensions I'm talking about are extensions to the SYCL library. None of them are extensions to the clang compiler proper. I don't suppose that changes your opinion on using Asciidoc? :-)
That makes sense, and it's aligned with my thinking also.
That's not how I read the website. The line for Asciidoc has a right-pointing arrow, which the legend defines as "conversion to". If they supported conversion both ways, I think that line would have a double-headed arrow. In addition, the release notes make several mentions of "asciidoc writer", but there is no mention of "asciidoc reader". For comparison, the release notes mention both "rst writer" and "rst reader".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more comment for now ... I'm not sure if this is a question to @sergey-semenov or to the group as a whole. How do we expect the upstreamed SYCL documentation to be organized? Looking at the LLVM download page, it seems like there are 7 top-level "entry points" for the documentation:
Are we thinking that SYCL would be an 8th top-level entry point? Or, would SYCL support be part of "clang"? For comparison, OpenMP documentation is under "clang", so it seems (to me) that it would make sense for SYCL documentation to be there also.
In order for this to work, would we move the SYCL documentation files in the repo to "clang/docs" instead of "sycl/doc"? Or, would we change the CMake rules to build the "clang" documentation from both the "clang/docs" and "sycl/doc" directories?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OpenMP provides compiler pragmas to access features, so it makes sense to have OpenMP documentation as part of the compiler documentation.
SYCL provides a library API, so I think adding another top-level entry fits better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does -- for the SYCL library documentation, I think MLIR is a reasonable precedent to point to for not using RST because it will be its own top-level project in the monorepo and has different needs. I was opposed to mixing and matching within Clang.
Ah, you may be right! Drat.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! It sounds like we may be converging on Option 4, then:
Is anyone opposed to this?
Still to do, then, is to add build rules to generate HTML from the Asciidoc files and integrate this into the top-level SYCL website.