-
Notifications
You must be signed in to change notification settings - Fork 633
Implementation of torch-to-linalg lowering of AtenOuterOp #4099
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
base: main
Are you sure you want to change the base?
Conversation
Hi, this is my first time contributing to the project - if you have any feedback or suggestions, I would really appreciate that. |
Thanks for picking this up. There isn't any reason to include quantization logic for this op since it doesn't have any qdq fusion implemented in It would also be a bit better to implement this directly as a Also, please do add e2e tests somewhere in |
cda896e
to
2348344
Compare
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.
After a change to the init tensor for the generic, I think this looks good!
Thanks for the changes.
Also, be sure to run either |
I changed it to the init tensor and ran pre-commit - everything looks good on my end. |
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.
Hi @amemov, can you please take a look at the CI failure?
Hi @vivekkhandelwal1, I skimmed it briefly before - I didn't see any failures specifically related to I will take a better look at it today, but so far I'm not really sure what exactly I need to modify / add here. |
Hi @amemov, some test(s) is/are crashing for the fx_importer config. Most probably, it will be the one that you have added. In order to find out which test is crashing you need to run the tests serially. You may use the following command:
The above command will run all the tests one by one. And, the last test run will be the one that's crashing. Then, you can figure out the fix for that. |
@vivekkhandelwal1
I resolved it by changing the casting and dimensions for operands. On my machine, it now passes AttenOuter test. |
@amemov, a contributor has added the lowering for this same op through decomposition here: #4138. Although your PR is old so in the case of conflict, you should get a chance to complete it, but their approach (via decomposition) is a better one. Can you and @ivanamitreski find a solution out of this? |
@amemov Please resolve the remaining comments. |
@zjgarvey could you take at the changes? Thank you! |
The tests are failing, so I'd recommend running your new tests locally with each of the different configs. |
@amemov I am part of a group which has adapted this conversion pattern as part of our lowering process for a MoE model down to the Linalg MLIR dialect. To fully integrate this pattern into our codebase, we need to pull it from the production Torch-MLIR. If you can address the test failures as suggested by the reviewer so it can be merged, we would greatly appreciate it. Thanks! |
@zjgarvey , I looked into the test failures and they're all unrelated to my AttenOuterOp changes. The 6 failing tests are all FX importer issues - nothing to do with the decomposition patterns I added. I verified my implementation works correctly by testing the decomposition manually as well, so I'm a little confused. I tried release and debug builds - both work on my end. Also, if you look at the failures in the CI it doesn't as well that the problem is due to the proposed decomposition |
Can you sync with main and we can rerun the Ci make sure it's not a failure specific to your changes? We won't be able to merge this unless the CI passes. |
- Defined the op in Linear.cpp TODO: - Testing, and perhaps add some test(-s) inside torch-mlir?
- Rewrote the ConvertAtenOuterOp without unsqueezing - Replaced linalg::MatmulOp with linalg::GenericOp for buidling result of the op - Added error messages for - Added test case in e2e tests - placed in matmul.py
8759641
to
f153429
Compare
@zjgarvey , I re-ran the tests on my machine after synching my branch with the main (and fetching these changes) - the errors I see are not due to AttenOuterOp lowering:
|
I think this is due to some missing build flags. I think you need to enable the jit IR importer and something else. The development.md doc should have some info about the cmake config for local testing. Then I'd do the |
@zjgarvey , I triple checked - all of my files are now in sync with the most recent changes. When I run Also for reference here is the build command I used - it includes everything to enable e2e testing:
|
An attempt to resolve #4093
Initial implementation: