Skip to content
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

Skip "find_split_transpose" if split has more than one outputs #2126

Merged
merged 3 commits into from
Sep 6, 2023

Conversation

umangyadav
Copy link
Member

@umangyadav umangyadav commented Aug 25, 2023

The current matcher is doing an assert on the number of outputs but instead, it should skip it if the number of outputs is greater than 1.

This matcher is doing

input --> slice --> transpose 
    | 
     ---->slice --> transpose 
    | 
    ---->slice --> transpose 

to

input --> transpose --> slice
                | 
                 ----> slice 
                | 
                ----> slice 

This is only useful if slice in original input graph has one output node (transpose in this case). If it has multiple outputs then doing transpose before the slice would change the layout for the slice's outputs (other than transpose) in original graph.

Partially solves #2116

@umangyadav umangyadav self-assigned this Aug 25, 2023
@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #2126 (6a8d74f) into develop (ac96370) will not change coverage.
Report is 1 commits behind head on develop.
The diff coverage is 100.00%.

❗ Current head 6a8d74f differs from pull request most recent head 09f35e7. Consider uploading reports for the commit 09f35e7 to get more accurate results

@@           Coverage Diff            @@
##           develop    #2126   +/-   ##
========================================
  Coverage    91.43%   91.43%           
========================================
  Files          422      422           
  Lines        15771    15771           
========================================
  Hits         14420    14420           
  Misses        1351     1351           
Files Changed Coverage Δ
src/simplify_algebra.cpp 96.79% <100.00%> (ø)

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Aug 25, 2023

Test Batch Rate new
09f35e
Rate old
ac9637
Diff Compare
torchvision-resnet50 64 2,282.83 2,281.62 0.05%
torchvision-resnet50_fp16 64 5,362.69 5,363.23 -0.01%
torchvision-densenet121 32 1,837.46 1,826.57 0.60%
torchvision-densenet121_fp16 32 3,390.99 3,387.24 0.11%
torchvision-inceptionv3 32 1,342.27 1,340.34 0.14%
torchvision-inceptionv3_fp16 32 2,592.18 2,582.06 0.39%
cadene-inceptionv4 16 680.23 682.15 -0.28%
cadene-resnext64x4 16 590.35 589.61 0.13%
slim-mobilenet 64 7,213.16 7,225.62 -0.17%
slim-nasnetalarge 64 236.67 236.89 -0.09%
slim-resnet50v2 64 2,527.47 2,527.05 0.02%
bert-mrpc-onnx 8 720.61 720.80 -0.03%
bert-mrpc-tf 1 390.69 389.58 0.28%
pytorch-examples-wlang-gru 1 302.21 301.37 0.28%
pytorch-examples-wlang-lstm 1 305.98 313.89 -2.52%
torchvision-resnet50_1 1 557.35 557.53 -0.03%
torchvision-inceptionv3_1 1 306.45 306.50 -0.02%
cadene-dpn92_1 1 351.99 345.97 1.74%
cadene-resnext101_1 1 220.90 220.27 0.28%
slim-vgg16_1 1 224.23 224.56 -0.15%
slim-mobilenet_1 1 1,476.56 1,478.76 -0.15%
slim-inceptionv4_1 1 223.35 222.77 0.26%
onnx-taau-downsample 1 247.37 247.83 -0.18%
dlrm-criteoterabyte 1 21.67 21.67 0.01%
dlrm-criteoterabyte_fp16 1 40.63 40.63 -0.02%
agentmodel 1 5,820.04 5,848.06 -0.48%
unet_fp16 2 55.06 55.06 0.01%

This build is OK for merge ✅

@migraphx-bot
Copy link
Collaborator


    :white_check_mark:bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

    :white_check_mark:bert-mrpc-tf: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

    :white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

🔴torchvision-inceptionv3_1: FAILED: MIGraphX is not within tolerance - check verbose output


🔴cadene-dpn92_1: FAILED: MIGraphX is not within tolerance - check verbose output


    :white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

🔴slim-inceptionv4_1: FAILED: MIGraphX is not within tolerance - check verbose output


    :white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

    :white_check_mark:agentmodel: PASSED: MIGraphX meets tolerance

    :white_check_mark:unet: PASSED: MIGraphX meets tolerance

@kahmed10
Copy link
Collaborator

I think we need to add a unit test. I've seen this happen in distilgpt2 as well.

@pfultz2
Copy link
Collaborator

pfultz2 commented Aug 31, 2023

I think we need to add a unit test. I've seen this happen in distilgpt2 as well.

Yea a unit test would be good.

@umangyadav
Copy link
Member Author

Added Unit-test

@umangyadav umangyadav requested a review from pfultz2 August 31, 2023 21:44
@causten causten merged commit 53c406b into develop Sep 6, 2023
12 checks passed
@causten causten deleted the fix_assumption branch September 6, 2023 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants