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

[TF:TRT] Support GatherV2 op when indices is constant #52268

Conversation

meena-at-work
Copy link
Contributor

@meena-at-work meena-at-work commented Oct 5, 2021

Convert the 'indices' input to a GatherV2 op into a constant layer when 'indices' is a constant.

Modify the unit tests to generate constant indices.

Signed-off-by: Meenakshi Venkataraman meenakshiv@nvidia.com

Tagging @bixia1 for review
CC: @tfeher @DEKHTIARJonathan

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Oct 5, 2021
@google-cla
Copy link

google-cla bot commented Oct 5, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Oct 5, 2021
@google-cla google-cla bot added the cla: no label Oct 5, 2021
@meena-at-work
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 5, 2021
@sanjoy sanjoy requested review from bixia1 and removed request for sanjoy October 6, 2021 05:12
@gbaned gbaned self-assigned this Oct 6, 2021
@gbaned gbaned added the comp:gpu:tensorrt Issues specific to TensorRT label Oct 6, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Oct 6, 2021
@bixia1
Copy link
Contributor

bixia1 commented Oct 6, 2021

In your PR description, you have:
Note: Support for this op in TRT implicit batch mode when both input and indices are both constants is limited to the case when batch size is 1.

we actually require batch size 1 when (1) both input and indices are tensors, or (2) both input and indices are constants.
But why do we add the case for (2), as we apply constant folding? In TF-TRT bridge, we report a problem when we see constant operations like this.

@meena-at-work
Copy link
Contributor Author

@bixia1 , regarding your comment:

we actually require batch size 1 when (1) both input and indices are tensors, or (2) both input and indices are constants.
But why do we add the case for (2), as we apply constant folding? In TF-TRT bridge, we report a problem when we see constant operations like this.

Thanks for pointing this out.

  1. is already covered in the converter -- and we flag it.
  2. I added support for with this patch. Previously we weren't supporting indices as constant at all, now we do.

I could remove my note on the PR, as it sounds redundant.

Copy link
Contributor

@bixia1 bixia1 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for your work!

@meena-at-work meena-at-work force-pushed the meenakshiv/gatherv2_converter branch 2 times, most recently from 556010c to 7a9ddb4 Compare October 7, 2021 20:26
@bixia1
Copy link
Contributor

bixia1 commented Oct 7, 2021

Looks good to me. I modified the PR description, please check.
Would you please squash your commits into one before I approve it? This is because the commit history will become part of the commit message if you don't squash the commits.

Convert 'indices' input to a GatherV2 op into a constant
layer if it is a constant.

Modify the unit tests to generate constant indices.

Signed-off-by: Meenakshi Venkataraman <meenakshiv@nvidia.com>
@meena-at-work
Copy link
Contributor Author

@bixia1 -- squash done.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 8, 2021
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Oct 8, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 8, 2021
@sherhut sherhut removed their request for review October 8, 2021 12:22
@gbaned gbaned removed the awaiting review Pull request awaiting review label Oct 8, 2021
@bixia1
Copy link
Contributor

bixia1 commented Oct 11, 2021

I am working on fixing some minor format issues and merging this PR from my end.

@copybara-service copybara-service bot merged commit d977c8f into tensorflow:master Oct 11, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:gpu:tensorrt Issues specific to TensorRT size:L CL Change Size: Large
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

4 participants