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.

Sign up for GitHub

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

Jump to bottom

fix: add an arg in matmul #2279

Merged
merged 2 commits into from
Sep 1, 2023
Merged

fix: add an arg in matmul #2279

merged 2 commits into from
Sep 1, 2023

Conversation

zewenli98
Copy link
Collaborator

Description

set_layer_name(layer, target, name) -> set_layer_name(layer, target, name, source_ir)

Fixes #2203

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project (You can use the linters)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas and hacks
  • I have made corresponding changes to the documentation
  • I have added tests to verify my fix or my feature
  • New and existing unit tests pass locally with my changes
  • I have added the relevant labels to my PR in so that relevant reviewers are notified

@github-actions github-actions bot added component: api [Python] Issues re: Python API component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths labels Aug 30, 2023
@github-actions github-actions bot requested a review from gs-olive August 30, 2023 01:11
gs-olive
gs-olive reviewed Aug 30, 2023
View reviewed changes
Copy link
Collaborator

@gs-olive gs-olive 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 - could you also add two arguments to matrix_multiply, which both default to trt.MatrixOperation.NONE, which allow us to control whether the matrix being multiplied gets transposed as well, so we can reduce operations required overall? They would control the variables input_matrix_op and other_matrix_op

@zewenli98
Copy link
Collaborator Author

Done! Thanks for the suggestion!

gs-olive
gs-olive reviewed Aug 30, 2023
View reviewed changes
py/torch_tensorrt/dynamo/conversion/aten_ops_converters.py Outdated
Comment on lines 189 to 190
args_bounds_check(args, 2, trt.MatrixOperation.NONE),
args_bounds_check(args, 3, trt.MatrixOperation.NONE),
Copy link
Collaborator

@gs-olive gs-olive Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You can remove these lines, since none of the converted Torch ops will have a third or fourth argument, based on their schemas.

The primary reason for adding those arguments to matrix_multiply below is that later, for aten.linear, you can pass in other_matrix_op=trt.MatrixOperation.TRANSPOSE for instance, and then reduce the number of layers (no need for a permute)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes sense! I've updated this PR and aten.linear in #2253.

@zewenli98
fix: improve matmul dynamo converter
b0d4c94 
fix bug

add args input_matrix_op and other_matrix_op, support aten.mv.default

minor fix
@zewenli98 zewenli98 mentioned this pull request Aug 30, 2023
Merged
7 tasks
gs-olive
gs-olive approved these changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@gs-olive gs-olive left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me, pending CI!

@gs-olive
Copy link
Collaborator

gs-olive commented Aug 31, 2023 •
edited
Loading

@zewenli98 - could you re-enable these tests for this PR, since it adds a converter for mv? It should just require enabling here:

TensorRT/tests/py/dynamo/conversion/test_matmul_aten.py

Lines 14 to 15 in e49ef6d

# FIXME torch.ops.aten.mv.default for (2,3), (3,1) - should mv be lowered to mm?
# (2,3), (3,) torch.ops.aten.mv.default

As well as here:

TensorRT/tests/py/dynamo/conversion/test_matmul_aten.py

Lines 44 to 45 in e49ef6d

# FIXME torch.ops.aten.mv.default for (2,3), (3,1) - should mv be lowered to mm?
# (2,3), (3,) torch.ops.aten.mv.default

You may need to add a new test class which looks for torch.aten.ops.mv.default.

@github-actions github-actions bot added the component: tests Issues re: Tests label Aug 31, 2023
@zewenli98
Copy link
Collaborator Author

Done! lmk if more changes are needed! Thanks!

gs-olive
gs-olive approved these changes Sep 1, 2023
View reviewed changes
Copy link
Collaborator

@gs-olive gs-olive 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!

@gs-olive gs-olive merged commit b1c7285 into pytorch:main Sep 1, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gs-olive gs-olive gs-olive approved these changes

Assignees
No one assigned
Labels
cla signed component: api [Python] Issues re: Python API component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths component: tests Issues re: Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose IMatrixMultiplyLayer in dynamo.conversion.impl
3 participants
@zewenli98 @gs-olive @facebook-github-bot