Repository navigation
[TF:TRT] Enable dynamic shape for "slice" and "strided slice" operations. - #51475
Conversation
22643fb to
de46f1e
Compare
|
@christopherbate is this ready to review, or is it still in a draft state? |
|
Still in draft, will be ready by end of week |
christopherbate
changed the title
christopherbate
changed the title
de46f1e to
072e889
Compare
|
@bixia1 This is ready for review |
ca28a20 to
6bfe721
Compare
|
I only reviewed part of the change in the tests and I am still working on reviewing the change to the converter. It would be nice if you can add some details to the PR description to describe the change in this PR, such as how you extend the converter to support dynamic shapes and the tests you added. |
|
@bixia1 I applied changes to address your comments as additional commits for readability, will squash once approved |
efe055d to
b8cdd0a
Compare
|
rebased on TOT and fixed preprocessor guard issue |
|
I added a commit to completely remove usage of TensorRT ISelectLayer as it is not supported with static version of TensorRT. Was able to work around it by replacing implementation of NonZero and AbsInt operations with slightly different implementations. |
| #include "tensorflow/core/platform/status.h" | ||
| #include "third_party/tensorrt/NvInfer.h" | ||
|
|
||
| // Use this macro within functions which return a Status or StatusOR<T> to check |
| return errors::Internal(__FILE__, ":", __LINE__, " TRT_ENSURE failure"); \ | ||
| } | ||
|
|
||
| // Check that a Status or StatusOr<T> object does not carry an error message. |
|
|
||
| // Check that a Status or StatusOr<T> object does not carry an error message. | ||
| // For pointer-containing StatusOr<T*>, use the below TRT_ENSURE_PTR_OK macro. | ||
| // If it does have an error, return an errors::Internal instance |
| } | ||
|
|
||
| // Check that a Status or StatusOr<T> object does not carry an error message. | ||
| // For pointer-containing StatusOr<T*>, use the below TRT_ENSURE_PTR_OK macro. |
There was a problem hiding this comment.
Let's move this line to the end of the comment block. Otherwise, the meaning of "it" in the the following sentence is not clear.
| x.status().ToString()); \ | ||
| } | ||
|
|
||
| // Check that a StatusOr<T*>object does not carry an error, and that the |
| this->Constant<int32>(-1, input->getDimensions().nbDims); | ||
| TRT_ENSURE_PTR_OK(neg_one); | ||
|
|
||
| // Make all negaitve elements into -1, positive and zero elements |
| this->Max(input, (*neg_one)->getOutput(0)); | ||
| TRT_ENSURE_PTR_OK(max); | ||
|
|
||
| // Make all positive elements into +1, negative and zero elements |
| // Adds an Add operation to the network. | ||
| StatusOr<nvinfer1::IElementWiseLayer*> Add(nvinfer1::ITensor* lhs, | ||
| nvinfer1::ITensor* rhs) noexcept { | ||
| TRT_ENSURE(network_); |
There was a problem hiding this comment.
My previous was shown as outdated, copied it back to here
Can we avoid TRT_EXPECT(network_) in all the functions that use it by making
"network_" "nvinfer1::INetworkDefinition* const network_" and only check it in the constructor?
There was a problem hiding this comment.
I don't think that check provides the same ability to catch mis-use of this class. It's definitely possible for the pointer to become null as it is not owned by the class.
There was a problem hiding this comment.
Sorry I don't understand what you are talking.
When we create TRTNetworkBuilder build(x, y);
build->network_ will get a copy of the pointer value and will stay the same unless a method of TRTNetworkBuilder modifies it. If we can make "nvinfer1::INetworkDefinition* const network_", we can kind of prevent this modification from happening.
Can you explain how build->network_ becomes null?
There was a problem hiding this comment.
Fixed, had a brain fart here ^^
There was a problem hiding this comment.
To test nullity of INetworkDefinition at construction, I made constructor private and added a public Create method which returns a StatusOr<TRTNetworkBuilder>
There was a problem hiding this comment.
Sorry, I don't see the change you said you have made. The constructor is still public, and I don't see the Create method. Also, all builder routines, such as Add and Min, still have the check. Did I miss something? @
| // functions, so this function multiplies the input by a zero value to side | ||
| // step knowing the shape. For static inputs, this can be optimized using one | ||
| // of the TRTNetworkBuilder::Constant function instead. | ||
| StatusOr<nvinfer1::IElementWiseLayer*> ZerosLikeDyamic( |
There was a problem hiding this comment.
I couldn't find any use of the routine.
Dynamic is mis-spelled as Dyamic.
This routine get a tensor with the same shape as the input and with value 0. It is hard to correlate its name ZerosLikeDynamic with its function. Maybe something like "ZerosWithShape"?
|
I went through once again and have pushed fixes for all comments minus the comment about scoping where I have asked for clarification on the style/rules. |
faf3f3e to
9a69cc6
Compare
google-ml-butler
Bot
added
kokoro:force-run
|
Googler refactored int64's to int64_t's, creating conflict just now. Do I need to rebase or do you guys handle this internal? |
|
Please rebase. |
9a69cc6 to
2b851df
Compare
|
rebased |
google-ml-butler
Bot
added
kokoro:force-run
gbaned
removed
awaiting review
google-ml-butler
Bot
added
kokoro:force-run
Enables dynamic shape support during conversion of Tensorflow "slice" and "strided slice" operations to TensorRT ISliceLayer.
Moves existing
StridedSliceHelperfunction to dedicatedops/slice_ops.[h|cc]file and refactors it extensively to support dynamic shape requirements.To support the creation of ancillary graphs in the TensorRT network, a helper class TRTNetworkBuilder is added to quickly create familiar operations (e.g. Nonzero, Sign, ZerosLike) via composition of those layers exposed by TensorRT.
Converter functions which utilize StridedSliceHlper (ConvertStridedSlice, ConvertSlice, ConvertSplit) are updated to use the new helper and gain dynamic shape support.
Tests for ConvertStridedSlice are updated to include better documentation and additional tests for dynamic shape. A negative test for new_axis_mask is added.