Revert find_reshape_cont guard relaxation from PR#4858#5052
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Regressions detected 🔴 |
|
ivarusic-amd
added
Windows
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…OCm/AMDMIGraphX into fix/revert-reshape-cont-guards
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| migraphx::make_op("multibroadcast", {{"out_lens", {2, 2, 2, 3}}}), trans_x); | ||
| auto sub = m2.add_instruction(migraphx::make_op("sub"), rsp_y, mb); | ||
| auto rsp_out = | ||
| m2.add_instruction(migraphx::make_op("reshape", {{"dims", {1, 1, 4, 3, 2}}}), sub); |
There was a problem hiding this comment.
The reshape should come before the sub.
There was a problem hiding this comment.
When i do this for the current pass with the changes in the if statements for this PR, the optimize_resize_ndims_unequal test fails so I'd have to change back everything for the reshape to come before the sub.
There was a problem hiding this comment.
Then removing the .lens() is not the right fix.
| // auto add_ins = m2.add_instruction(migraphx::make_op("add"), y_trans, x_brcst); | ||
| // auto r = m2.add_instruction(migraphx::make_op("reshape", {{"dims", {2, 4, 6}}}), | ||
| // add_ins); m2.add_return({r}); | ||
| // } |
There was a problem hiding this comment.
We shouldnt be disabling the tests.
There was a problem hiding this comment.
That one was failing with the old changes. It was added in the PR where the guards were originally added. #4858
There was a problem hiding this comment.
We shouldn't disable a test though - we can modify them if say the IR has changed but removing this isn't the right call since it ensures other runs/models using this matcher won't behave the same now.
|
Can you add the test case where |
add a test case for "customer model usecase "? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5052 +/- ##
===========================================
+ Coverage 92.82% 92.89% +0.06%
===========================================
Files 597 603 +6
Lines 32145 32449 +304
===========================================
+ Hits 29838 30141 +303
- Misses 2307 2308 +1
🚀 New features to boost your workflow:
|
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I think he means to distill the part of the model into a unit test case that can act as a minimum repro of the scenario. There are tools to slice onnx models or just generally construct some tensors/ops in a unit test that reproduce the failing case if you know what the failing graph structure is like. |
@tperry-amd @tamahedi correct - we have examples of this in simplify_reshapes_test If this is related to say some sort of onnx input or model you can add or generate the exact onnx using our gen_onnx.py script.- thats how all our onnx tests are generated. Regardless you'll need to add the case you're seeing here, parse it to ensure no issues with parser and then create a new unit test in simpify_reshapes_test.cpp. If its not something within the operator but a pattern you're seeing reproduce it as MIGraphX IR If its just a pattern you're trying to optimize - reproduce that into the new unit test you'll create in simplify_reshapes_test.cpp Then you can build MIGraphX and run Once that passes without regressions then run `test_optimize_module_test -c' this runs simplify_reshapes + simplify algebra and checks some patterns that use both matchers to ensure consistency and expected output. A finally a sanity would be Ensure you do this for a Release build flag, and Debug build flag incase any static_asserts trigger - CI will test this anyway (this is what our debug builds do) but its better to catch this early since you can test/verify everything locallt with the binary tests. If you're on linux you can use If you want to regen tests quickly you can also build |
5 participants
Motivation
There is this ticket right now: AIRADSW-665. When I did a regression search for it I found it came from migraphx and this PR #4858. After going through each changes in that PR I found the bug came from the changes in src/simplify_reshapes.cpp. I reverted those changes here in this PR.
Technical Details
src/simplify_reshapes.cpp has had its guards find_reshape_cont struct reverted to more strict checking because it allowed some checks on strides to go through that were causing TDR.
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable