-
Notifications
You must be signed in to change notification settings - Fork 499
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
Adding reshape fall back case for unimplemented cases in aten view for time being #2515
Conversation
op, "unimplemented: input rank 0 is not supported"); | ||
// return rewriter.notifyMatchFailure( | ||
// op, "unimplemented: input rank 0 is not supported"); | ||
rewriter.replaceOpWithNewOp<tensor::ReshapeOp>(op, resultType, input, sizeTensor); |
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.
tensor::ReshapeOp
is not a good op to lower to. See discussion here: #1168
Do you have examples of the cases you need to handle that currently fail? I can help brainstorm approaches
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.
Others (Gaurav, Mahesh) are working on another approach with collapsing and expanding. This is just a short term fix until we get that patch landed. Specifically, we are trying to lower llama and currently blocked. I'll add the specific issue to this thread once I get it.
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.
Llama_2_7b_chat_hf_torch.mlir:958:10: error: failed to legalize operation 'torch.aten.view' that was explicitly marked illegal
%5 = torch.aten.view %3, %4 : !torch.vtensor<[1,?],si64>, !torch.list<int> -> !torch.vtensor<[1,?],si64>
^
Llama_2_7b_chat_hf_torch.mlir:958:10: note: see current operation: %55 = "torch.aten.view"(%53, %54) : (!torch.vtensor<[1,?],si64>, !torch.list<int>) -> !torch.vtensor<[1,?],si64>
Llama_2_7b_chat_hf_torch.mlir:12546:10: error: failed to legalize operation 'torch.aten.view' that was explicitly marked illegal
%6 = torch.aten.view %4, %5 : !torch.vtensor<[1,?],si64>, !torch.list<int> -> !torch.vtensor<[1,?],si64>
^
Llama_2_7b_chat_hf_torch.mlir:12546:10: note: see current operation: %69 = "torch.aten.view"(%67, %68) : (!torch.vtensor<[1,?],si64>, !torch.list<int>) -> !torch.vtensor<[1,?],si64>
I think this is the same issue that I solved locally by adding that logic that you were asking about in #2485
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.
its still weird to me that this fails. Let me double check I'm on main too
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.
yeah does still happen ToM,
https://storage.googleapis.com/shark_tank/dan/repro.mlir
https://storage.googleapis.com/shark_tank/dan/repro_readable.mlir
iree-compile --iree-input-type=torch repro.mlir
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 would be good to know what is blocking things downstream. @dan-garvey mentioned above that the issue might be fixed by making a small change to how the output shape is calculated. If so, this would be preferred to adding a new op that downstream projects have to handle. A small reproducer of what needs solving would be very useful here.
If the issue turns out to be very hard to handle given the current setup, then the plan you've outlined makes sense.
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.
yeah so I have a branch that solves all the issues without resorting to reshape. I'll have to make the small repro for the sake of tests for that patch anyway, so I think we can close this for now.
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.
I'll aim to have that up for re"view" today
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.
Perfect! Thanks @dan-garvey
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.
Closing this PR for now then.
No description provided.