Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Docs update: how to use dynamic shared mem with block reduce #348

Merged
merged 9 commits into from
Aug 18, 2021

Conversation

MatthiasKohl
Copy link
Contributor

As mentioned in #344, I created this PR to add a simple example of how to correctly use dynamic shared memory with BlockReduce.

I hope I followed the PR process correctly, I wasn't able to re-build the docs because the full build (for thrust) took longer than my allocated time on the machine when I was working on this, and I'm not familiar enough with the build process s.t. I can simply only build the docs for cub.
I'll amend the pull request, of course, if needed.

Plus, do you want me to add a similar example to classes like BlockScan ? In this case, it may be better to add an actual example file and link to it from the various inline docs.

@alliepiper
Copy link
Collaborator

I wasn't able to re-build the docs

No problem, our docs framework is a work-in-progress. This should improve soon.

it may be better to add an actual example file and link to it from the various inline docs.

I agree, this would be better to have a complete example under the examples/ directory. We'll build it as part of our CI to make sure it keeps working this way, too. Can you add this?

@alliepiper alliepiper added this to the 1.14.0 milestone Jul 19, 2021
@alliepiper alliepiper added only: docs Documentation changes only. Doesn't need code CI. only: tests Changes to tests only. P2: nice to have Desired, but not necessary. labels Jul 19, 2021
@MatthiasKohl
Copy link
Contributor Author

I agree, this would be better to have a complete example under the examples/ directory. We'll build it as part of our CI to make sure it keeps working this way, too. Can you add this?

Sure, I'll add it as soon as I get some time for it (probably tomorrow).

@MatthiasKohl
Copy link
Contributor Author

Sorry for the delay, but I finally got to doing this and added the full example.
I can see that the example compiles (and tests pass), but I'm not sure what the best way to integrate it into the docs is, yet.
Especially since I wasn't able to compile the docs.
Would it be possible for you to check that? It may also make sense to link to this example from other block-wide operations since they'll work basically the same w.r.t. shared memory

@alliepiper alliepiper self-assigned this Jul 30, 2021
@alliepiper
Copy link
Collaborator

Unfortunately, our docs build is broken at the moment, so we can't really test this out just yet. NVIDIA/thrust#1475 is working on restoring it.

It might be best to just mention the name of the example without a link. Doxygen provides the \ref command to create internal links, but I don't see how to get that to link to a complete source file, it appears to only work with documentation blocks.

It may also make sense to link to this example from other block-wide operations since they'll work basically the same w.r.t. shared memory

Agreed -- could you update this PR to do so?

@MatthiasKohl
Copy link
Contributor Author

Yes, I slightly reformulated the docs such that it should be clear where to find the example in case the tags don't work.
Although I believe they should work with doxygen and I've seen other projects using them (they are even supposed to be automatically substituted with references if you build the docs to LaTeX, but I'll believe it when I see it...).

And I added this example along with the short description to all block-wide operations except for BlockAdjacentDifference which doesn't seem to have any docs for now, and BlockShuffle which has very minimal docs.

@alliepiper
Copy link
Collaborator

@senior-zero is currently working on BlockAdjacentDifference -- George, can you include similar wording as the new docs here?

I should be able to get this merged in the next few days.

alliepiper added a commit to alliepiper/thrust that referenced this pull request Aug 17, 2021
@alliepiper
Copy link
Collaborator

Made some minor modifications. Started tests in NVIDIA/thrust#1504.

@alliepiper alliepiper added the testing: gpuCI in progress Started gpuCI testing. label Aug 17, 2021
@alliepiper alliepiper mentioned this pull request Aug 17, 2021
@alliepiper alliepiper added testing: gpuCI passed Passed gpuCI testing. and removed testing: gpuCI in progress Started gpuCI testing. labels Aug 18, 2021
@alliepiper alliepiper merged commit 772eae8 into NVIDIA:main Aug 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
only: docs Documentation changes only. Doesn't need code CI. only: tests Changes to tests only. P2: nice to have Desired, but not necessary. testing: gpuCI passed Passed gpuCI testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants