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

Use DynamicScratchpad in KernelManager. #3670

Merged
merged 7 commits into from
Feb 15, 2022

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Feb 10, 2022

Signed-off-by: Michał Zientkiewicz [email protected]

Category:

New feature (non-breaking change which adds functionality)

Description:

The dynamic scratchpad is now the primary scratchpad implementation.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: DALI-2449

Signed-off-by: Michał Zientkiewicz <[email protected]>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3934559]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3934559]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3935973]: BUILD STARTED

@mzient mzient force-pushed the UseDynamicScratchpad branch from e32b71d to 900733a Compare February 10, 2022 18:36
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3936330]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3936330]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3943354]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3943354]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3957968]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3959429]: BUILD STARTED

@mzient mzient force-pushed the UseDynamicScratchpad branch from 8252a74 to 1ef772c Compare February 14, 2022 15:07
…d, Convolution and Laplacian tests.

Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient mzient force-pushed the UseDynamicScratchpad branch from 1ef772c to 4c7e414 Compare February 14, 2022 15:33
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3959606]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3959606]: BUILD PASSED

@mzient mzient marked this pull request as ready for review February 15, 2022 10:44
@NVIDIA NVIDIA deleted a comment from dali-automaton Feb 15, 2022
@JanuszL JanuszL self-assigned this Feb 15, 2022
"Kernel instance index (instance_idx) out of range");
auto &inst = instances[instance_idx];
DynamicScratchpad scratchpad({}, AccessOrder(context.gpu.stream));
auto *old_scratchpad = context.scratchpad;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need context.scratchpad for if we are not using it anyway here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used by the Run method invoked afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was rather asking about the ideas of replacing the old one with one created here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, now the old one is always NULL, so we don't need to store it. I'll simplify it (again, possibly in a follow-up).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let do that in a follow-up.

Copy link
Member

Choose a reason for hiding this comment

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

So kernel manager scratchpads are also to be removed in a follow-up, right?

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, that's what I brought up in open floor. Since this involves an API change (we no longer need thread_idx), the change affects a large number of files - hence the decision to make the more important change that potentially affects performance (this one) first and then clean the rest.

@mzient mzient mentioned this pull request Feb 15, 2022
18 tasks
Signed-off-by: Michal Zientkiewicz <[email protected]>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3968027]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3968027]: BUILD PASSED

@mzient mzient merged commit 720f987 into NVIDIA:main Feb 15, 2022
cyyever pushed a commit to cyyever/DALI that referenced this pull request Feb 21, 2022
* Use DynamicScratchpad in KernelManager.
* Use invalid stream as a default for KernelContext.
* Lazily initialize upstream resources for dynamic scratchpad.
* Do not initialize the underlying resource for DynamicScratchpad when allocating 0 bytes.

Signed-off-by: Michal Zientkiewicz <[email protected]>
@JanuszL JanuszL mentioned this pull request Mar 30, 2022
cyyever pushed a commit to cyyever/DALI that referenced this pull request May 13, 2022
* Use DynamicScratchpad in KernelManager.
* Use invalid stream as a default for KernelContext.
* Lazily initialize upstream resources for dynamic scratchpad.
* Do not initialize the underlying resource for DynamicScratchpad when allocating 0 bytes.

Signed-off-by: Michal Zientkiewicz <[email protected]>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jun 7, 2022
* Use DynamicScratchpad in KernelManager.
* Use invalid stream as a default for KernelContext.
* Lazily initialize upstream resources for dynamic scratchpad.
* Do not initialize the underlying resource for DynamicScratchpad when allocating 0 bytes.

Signed-off-by: Michal Zientkiewicz <[email protected]>
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.

4 participants