-
Notifications
You must be signed in to change notification settings - Fork 364
Adding support for aten::topk #302
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
1557f6e
support topk converter/test_case
inocsin 22e6a6b
feat(aten::topk): Adding BUILD files for topk op
narendasan 5bd7528
refactor(aten::topk): linting topk converter
narendasan 81f1e9d
feat(aten::topk): Add a debug message noting that sorted is always true
narendasan 61661ff
refactor(aten::topk): apply linting to topk
narendasan 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 hidden or 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 hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
#include "NvInfer.h" | ||
#include "core/conversion/converters/converters.h" | ||
#include "core/conversion/tensorcontainer/TensorContainer.h" | ||
#include "core/util/prelude.h" | ||
#include "torch/torch.h" | ||
|
||
#include <ATen/ATen.h> | ||
#include <vector> | ||
|
||
namespace trtorch { | ||
namespace core { | ||
namespace conversion { | ||
namespace converters { | ||
namespace impl { | ||
namespace { | ||
|
||
auto topk_registrations TRTORCH_UNUSED = RegisterNodeConversionPatterns().pattern( | ||
{"aten::topk(Tensor self, int k, int dim=-1, bool largest=True, bool sorted=True) -> (Tensor values, Tensor indices)", | ||
[](ConversionCtx* ctx, const torch::jit::Node* n, args& args) -> bool { | ||
auto self = args[0].ITensorOrFreeze(ctx); | ||
auto k = args[1].unwrapToInt(); | ||
auto dim = args[2].unwrapToInt(); | ||
auto largest = args[3].unwrapToBool(); | ||
LOG_DEBUG( | ||
"Note: sorted argument is not used in TensorRT for aten::topk, results will depend on the value of largest"); | ||
// auto sorted = args[4].unwrapToBool(); # Currently unused | ||
|
||
auto selfDim = util::toVec(self->getDimensions()); | ||
|
||
// reduceAxes The reduction dimensions. The bit in position i of bitmask reduceAxes corresponds to explicit | ||
// dimension i of the result. E.g., the least significant bit corresponds to the first explicit dimension and the | ||
// next to least significant bit corresponds to the second explicit dimension. | ||
|
||
if (dim < 0) { | ||
dim = selfDim.size() + dim; | ||
} | ||
|
||
uint32_t shiftDim = 1 << dim; | ||
|
||
LOG_DEBUG("Output topk reduce dim: " << dim); | ||
|
||
auto TopKOperation = largest ? (nvinfer1::TopKOperation::kMAX) : (nvinfer1::TopKOperation::kMIN); | ||
|
||
auto new_layer = ctx->net->addTopK(*self, TopKOperation, k, shiftDim); | ||
|
||
TRTORCH_CHECK(new_layer, "Unable to create topk layer from node: " << *n); | ||
|
||
auto out0 = ctx->AssociateValueAndTensor(n->outputs()[0], new_layer->getOutput(0)); | ||
auto out1 = ctx->AssociateValueAndTensor(n->outputs()[1], new_layer->getOutput(1)); | ||
|
||
LOG_DEBUG("Output tensor(0) shape: " << out0->getDimensions()); | ||
LOG_DEBUG("Output tensor(1) shape: " << out1->getDimensions()); | ||
|
||
return true; | ||
}}); | ||
|
||
} // namespace | ||
} // namespace impl | ||
} // namespace converters | ||
} // namespace conversion | ||
} // namespace core | ||
} // namespace trtorch |
This file contains hidden or 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 hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
#include <string> | ||
#include "core/compiler.h" | ||
#include "gtest/gtest.h" | ||
#include "tests/util/util.h" | ||
#include "torch/csrc/jit/ir/irparser.h" | ||
|
||
TEST(Converters, ATenTopKConvertsCorrectly) { | ||
const auto graph = R"IR( | ||
graph(%0 : Tensor): | ||
%1 : int = prim::Constant[value=20]() | ||
%2 : int = prim::Constant[value=-1]() | ||
%3 : bool = prim::Constant[value=1]() | ||
%4 : bool = prim::Constant[value=1]() | ||
%5 : Tensor, %6 : Tensor = aten::topk(%0, %1, %2, %3, %4) | ||
return (%5, %6))IR"; | ||
|
||
auto g = std::make_shared<torch::jit::Graph>(); | ||
torch::jit::parseIR(graph, &*g); | ||
|
||
auto in = at::rand({10, 10, 100}, {at::kCUDA}); | ||
|
||
auto params = trtorch::core::conversion::get_named_params(g->inputs(), {}); | ||
auto jit_results = trtorch::tests::util::RunGraph(g, params, {in}); | ||
|
||
params = trtorch::core::conversion::get_named_params(g->inputs(), {}); | ||
auto trt_results = trtorch::tests::util::RunGraphEngine(g, params, {in}); | ||
|
||
ASSERT_TRUE(trtorch::tests::util::almostEqual(jit_results[0], trt_results[0].reshape_as(jit_results[0]), 2e-6)); | ||
ASSERT_TRUE(trtorch::tests::util::almostEqual(jit_results[1], trt_results[1].reshape_as(jit_results[1]), 2e-6)); | ||
} |
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.
@inocsin What is the role of this argument and why don't we use it? I commented it out because it was throwing warnings
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.
torch.topk has an argument sorted = True which returns the top k elements in a sorted order (decreasing). By default it is true. Setting it False will result in random order of output top k elements which we can't support in TensorRT. TensorRT always returns the top "k" max or min elements depending on "largest" argument. Maybe we can add this detail as a comment to explain why we aren't using args[4]
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.
will this cause an issue to users? if they expect random does always giving them ordered matter? At the very least yeah there should be a comment/
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.
This won't cause an issue to users in my opinion. So far all usecases for this op have been gathering either top k largest elements or top k smallest elements, both of which are supported. We can revisit this if anyone requests such usecase.