-
Notifications
You must be signed in to change notification settings - Fork 143
Small optimizations #768
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
base: master
Are you sure you want to change the base?
Small optimizations #768
Changes from 8 commits
3c0cb25
396f6cf
23d3038
6c11ba8
7eaa40b
c52c53b
359430c
342dc8f
919bcbe
160f0c8
0caca66
c2537c1
8e1c35e
f501eab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,12 @@ | ||
| /* Part of this file was contributed by NVIDIA under license: | ||
| * Copyright (C) 2020 NVIDIA Corporation | ||
| * SPDX-License-Identifier: MIT | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include "common/definitions.h" | ||
| #include "common/shape.h" | ||
| #include "marian.h" | ||
|
|
||
| #include <iostream> | ||
|
|
@@ -12,23 +19,22 @@ struct State { | |
| Expr output; | ||
| Expr cell; | ||
|
|
||
| State select(const std::vector<IndexType>& selIdx, // [beamIndex * activeBatchSize + batchIndex] | ||
| State select(Expr selIdx, // [beamIndex * activeBatchSize + batchIndex] | ||
| int beamSize, bool isBatchMajor) const { | ||
| return{ select(output, selIdx, beamSize, isBatchMajor), | ||
| select(cell, selIdx, beamSize, isBatchMajor) }; | ||
| } | ||
|
|
||
| // this function is also called by Logits | ||
| static Expr select(Expr sel, // [beamSize, dimTime, dimBatch, dimDepth] or [beamSize, dimBatch, dimTime, dimDepth] (dimTime = 1 for RNN) | ||
| const std::vector<IndexType>& selIdx, // [beamIndex * activeBatchSize + batchIndex] | ||
| Expr selIdx, // [beamIndex * activeBatchSize + batchIndex] | ||
| int beamSize, bool isBatchMajor) | ||
| { | ||
| if (!sel) | ||
| return sel; // keep nullptr untouched | ||
|
|
||
| sel = atleast_4d(sel); | ||
|
|
||
| int dimBatch = (int)selIdx.size() / beamSize; | ||
| int dimBatch =(int) selIdx->shape().elements()/beamSize; | ||
| int dimDepth = sel->shape()[-1]; | ||
| int dimTime = isBatchMajor ? sel->shape()[-2] : sel->shape()[-3]; | ||
|
|
||
|
|
@@ -83,8 +89,24 @@ class States { | |
| States select(const std::vector<IndexType>& selIdx, // [beamIndex * activeBatchSize + batchIndex] | ||
| int beamSize, bool isBatchMajor) const { | ||
| States selected; | ||
| Expr indices; | ||
| // I think this doesn't work if model split among gpus but not sure if it matters | ||
|
|
||
| for (auto& state : states_) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment what this logic does, as it is not obvious why the old code is not working.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is nothing wrong with the old code. This just needs to check if we need to ship indices to the GPU. I will add a comment explaining what this does.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a comment in a recent commit but I'm not sure if it's clear enough. If so, feel free to resolve this. |
||
| if (state.cell) { | ||
| indices = state.cell->graph()->indices(selIdx); | ||
| break; | ||
| } | ||
|
|
||
| if (state.output) { | ||
| indices = state.output->graph()->indices(selIdx); | ||
| break; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if neither? Is that a valid condition? If not, let's change this to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think neither is a valid condition
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, because it's a loop, sorry. But what happens if it never matches any of the conditions? Then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I think |
||
| } | ||
|
|
||
| // GPU OPT: Implement kernel to batch these on GPU | ||
| for(auto& state : states_) | ||
| selected.push_back(state.select(selIdx, beamSize, isBatchMajor)); | ||
| selected.push_back(state.select(indices, beamSize, isBatchMajor)); | ||
| return selected; | ||
| } | ||
|
|
||
|
|
||
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.
What is this for? It seems the only change is the rounding of
maxDims. What NVidia contribution was made here?In general, I would be opposed to changing the comment style for licence. Marian source code does not have license information in the source files directly, but rather in a separate license file. Please let's continue to follow that pattern.
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 that was the only contribution. However, I was told to include a notice even in files where I make one line changes. This is just me following instructions.
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.
Also, I just saw the second part of your comment. Is there a process for adding NVIDIA to the license file? I'm not sure what the solution is here.
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 one that hasn't been updated since 2016 and doesn't even name Microsoft? I guess add it to your PR and shame Marcin.
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.
@kpu I think this is the one Frank was referring to. I'll ask Marcin if this is ok. I will also need to check to see if we are internally ok with removing the notices assuming we are added to the license file.
@emjotde What do you suggest as the way forward?
Uh oh!
There was an error while loading. Please reload this page.
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 checked internally and I can add NVIDIA to the main license file and remove the notices in all the other files!
I will take care of this in all the PRs I have submitted.
Edit: Let me know if the license change looks ok.