-
Notifications
You must be signed in to change notification settings - Fork 487
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
Added e2e LTC tests #916
Merged
henrytwo
merged 13 commits into
llvm:torch_mlir_ltc_backend
from
henrytwo:henrytu/ltc_tests
Jun 9, 2022
Merged
Added e2e LTC tests #916
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
a4382a0
Added e2e LTC Torch MLIR tests
henrytwo ef3f325
Fix seed for reproducability
henrytwo 4f7f999
Check if computation is None before getting debug string
henrytwo 9546f10
Updated unit tests, and added numeric tests
henrytwo e76464b
Print name of the model layer that fails numeric validation
henrytwo d00f8c7
Run LTC e2e test with CI/CD
henrytwo f75c473
Set seed in main function, instead of beginning of execution
henrytwo 28c50e0
Add comment to specify number of digits of precision
henrytwo 0b737f7
Fixed typo
henrytwo 9e23d97
Remove tests for LTC example models
henrytwo 923a9fe
Added LTC option to torchscript e2e
henrytwo 890979c
Implement compile and run for LTC e2e test
henrytwo 08101eb
xfail all tests that use ops that aren't currently supported
henrytwo 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 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 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 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
Oops, something went wrong.
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.
Any triage on why this could be failing?
If we are running the JIT graph directly, I would assume that everything "Just Works".
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.
We don't have support for all ops yet. Some are missing some shape inference functions, some ops are blacklisted, etc.
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.
Ah, got it. It's a little annoying that we are duplicating the shape functions for LTC. Could you collaborate with @eellison to reuse their shape stuff?
We have been upstreaming all our shape functions from https://github.com/llvm/torch-mlir/blob/main/python/torch_mlir/dialects/torch/importer/jit_ir/build_tools/shape_lib_gen.py, so this should all be part of a big source of truth upstream. E.g. pytorch/pytorch#76889
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.
Hm our shape inferencing happens in C++, so I don't think there's much we can do as far as reusing that code 😢
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.
All of the shapes functions are also stored in C++ fwiw, so I don't think that's a fundamental blocker. but I dont know all the details of what's going on 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.
@eellison Can you point me to where that is in the PyTorch repo? I just looked at the PR that Sean linked earlier, which had C++ with some long strings.
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.
It should turn into JIT IR (that's what you see in the strings) that we can then load up and execute from C++.
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.
@silvasean are you suggesting that we apply this at the JIT graph level, or port this over to work with LTC generally?
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.
If it's the former, I don't think we can use it, because we require shape information at the LTC layer before generating a JIT graph.
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.
Oh, interesting. Yeah, that makes things more tricky to reuse. @eellison is the shape inference infra tied specifically to JIT graphs?
I suppose we could create a shapeless jit graph just for the purpose of reusing the shape inference infrastructure, but that's quite a few hoops to jump through.