Page MenuHomePhabricator

Unify build documentation and convert to reStructuredText
ClosedPublic

Authored by Hahnfeld on Dec 6 2017, 1:51 PM.

Details

Summary

We now have several options that apply for both libraries and they
shouldn't be documented in multiple files. When already merging
the two Build_With_CMake.txt documents, convert them to
reStructuredText which is used for all of LLVM's documentation.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Dec 6 2017, 1:51 PM

This is a first version, I will do another pass tomorrow. Proof-reading would be much appreciated :-)

grokos added inline comments.Dec 6 2017, 6:26 PM
README.rst
40 ↗(On Diff #125803)

all values (remove "the")

186 ↗(On Diff #125803)

What is that question mark?

231 ↗(On Diff #125803)

assembly --> assembler?

256–258 ↗(On Diff #125803)

We had an internal discussion about it. Do we have consensus that we'll support bclib?

281 ↗(On Diff #125803)

I think we should we also include something like:

**LIBOMPTARGET_NVPTX_DEBUG**
Enable printing of debug messages from the nvptx runtime.

Following you comments on the nvptx patch, I added this entry in my local branch and I was planning to include it in the next diff. You can add it in this patch instead. However, there are no ON or OFF options, the symbol is either defined or not defined.

317–318 ↗(On Diff #125803)

Remove commas before "and" (2 places).

grokos added inline comments.Dec 6 2017, 6:41 PM
README.rst
277 ↗(On Diff #125803)

The default is still sm_30, let's patch clang first to use sm_35. Also, given that the nvptx runtime cannot be built with sm<35, should we add to the description that sm_35 is the minimum required?

Hahnfeld updated this revision to Diff 125947.Dec 7 2017, 6:53 AM
Hahnfeld marked 4 inline comments as done.

Improvements based on review comments and extensions to some paragraphs.

README.rst
256–258 ↗(On Diff #125803)

For now, I've only copied what has been in libomptarget/Build_With_CMake.txt. To be honest I think we should remove this paragraph entirely because the NVPTX runtime hasn't been committed yet so that we don't confuse the users.

317–318 ↗(On Diff #125803)

Some of the comas are intentional to be consistent with the rest of LLVM's documentation: https://en.wikipedia.org/wiki/Serial_comma

Hahnfeld updated this revision to Diff 127852.Dec 21 2017, 3:02 AM

Removed documentation about the NVPTX runtime for now which isn't committed yet.

Ping. Is this ok to land before the 6.0 branch?

omalyshe requested changes to this revision.Dec 21 2017, 4:21 AM
omalyshe added a subscriber: omalyshe.
omalyshe added inline comments.
README.rst
141 ↗(On Diff #127852)

The correct name is "Intel® Many Integrated Core Architecture (Intel® MIC Architecture)".

153 ↗(On Diff #127852)

Linux*

154 ↗(On Diff #127852)

Windows*
macOS*

168 ↗(On Diff #127852)

macOS*
and the same in the next line OS X* ->macOS*

195 ↗(On Diff #127852)

Probably it's worth to add "Include support for the OpenMP Tools Interface (OMPT)"

This revision now requires changes to proceed.Dec 21 2017, 4:21 AM
Hahnfeld updated this revision to Diff 127890.Dec 21 2017, 7:54 AM
Hahnfeld marked 5 inline comments as done.

Address review comments and pay attention to protected names.

This revision is now accepted and ready to land.Dec 21 2017, 10:30 PM
This revision was automatically updated to reflect the committed changes.