-
Notifications
You must be signed in to change notification settings - Fork 534
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
[LINALG][MLIR] Fix the broadcast dim check for elementwise ops #2882
base: main
Are you sure you want to change the base?
[LINALG][MLIR] Fix the broadcast dim check for elementwise ops #2882
Conversation
Signed-Off-by: Gaurav Shukla <[email protected]>
5bb719e
to
b3dfea2
Compare
@stellaraccident @rsuderman Let me know if this sounds good to you, I will add a test case and update the existing test cases. Thanks! |
Could we validate with a test that the broadcast case would pass? I am a little concerned that removing the assertion will just cause runtime failures. |
Suggest creating few Operator level tests and adding to https://github.com/nod-ai/SHARK-TestSuite/tree/main/e2eshark |
The problem is that I don't think it can be dynamically 1. That is illegal at the pytorch level. (The assert is correct) |
I think it is likely that the problem is leading up to this and we are not narrowing it to a static value. You can't dynamically switch between an expanding and non expanding dim. |
I think it is likely that the problem is leading up to this and we are not narrowing it to a static value. You can't dynamically switch between an expanding and non expanding dim. These are abnormally small tensors and are raising red flags for me. Given that this is only 47 instructions deep, can you post the ir ave function declaration up to this point? |
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.
Marking request changes because the assert is correct and we need to chase this further.
It's not exactly 47th instruction in the opt-125m model, we got a smaller IR out of opt model to reproduce the issue.
|
I guess, folders might be helpful here. |
Maybe extending the logic in |
That IR needs some more work... One thing that jumps out to me: you've got multiple onnx.Constant ops that are just capturing i1 0d tensors as dense resources. Those are probably feeding in to conditionals that are constraining the entire thing. I doubt know why those are coming in like that but they will block any kind of folding, even if it existed. I have a gut feeling that this is doing a lot of dumb "shape like" work on those small tensors, probably just to be thrown out. This old onnx and tf gunk was lossy with that sort of thing. But you've got to get the ir normalized and folded a bit to weed it out. Those onnx.Constant ops shouldn't be there and they should be coming in as dense_elements with a single value so they can be analyzed. I'd need to stare at the whole problem some more to have better guidance. |
There's probably one use of that whole subgraph that is conditioned on some constant or input shape thing... The onnx representation is really bad for that stuff and we'll need to implement sufficient simplifications. I was there when the tflite folks had to do the same with insane tf graphs, which basically look like this :/ |
Before boiling the ocean on this, I'd recommend using a third party tool to simplify the onnx graph. Example: https://github.com/daquexian/onnx-simplifier |
No description provided.