Skip to content

Conversation

@vinx13
Copy link
Member

@vinx13 vinx13 commented Nov 26, 2018

Fix the issue mentioned in #2163

  • It is possible that conv2d has parallel branches that are not conv2d, so a condition check is added.
  • When grouping branches, ensure branches with different root to be placed in different groups (even if they are compatible)

Please review @masahi @tqchen

@vinx13 vinx13 force-pushed the fix/combine_parallel_conv2d branch from 55cc209 to fc53ed6 Compare November 26, 2018 02:40
@masahi
Copy link
Member

masahi commented Nov 26, 2018

even with this change, VGG and Resnet tests in tests/python/frontend/mxnet/test_forward.py don't work if I enable CombineParallelConv2D pass. @vinx13 can you take a look?

The weird thing is that I am getting slightly different errors every time I run test_forward_vgg() and test_forward_resnet().

@masahi
Copy link
Member

masahi commented Nov 26, 2018

@vinx13 is CombineParallelConv2DPass supposed to change residual block? When I run test_forward_resnet(), it sometimes finishes without error, but I am seeing a log like the one below. This is not a standard workload in resnet.

WARNING:autotvm:Cannot find config for target=llvm, workload=('conv2d', (1, 128, 28, 28, 'float32'), (2944, 128, 3, 3, 'float32'), (1, 1), (1, 1), (1, 1), 'NCHW', 'float32'). A fallback configuration is used, which may bring great performance regression.
WARNING:autotvm:Cannot find config for target=llvm, workload=('conv2d', (1, 64, 56, 56, 'float32'), (896, 64, 1, 1, 'float32'), (2, 2), (0, 0), (1, 1), 'NCHW', 'float32'). A fallback configuration is used, which may bring great performance regression.

@vinx13
Copy link
Member Author

vinx13 commented Nov 26, 2018

@masahi It shouldn't. Fixed, please review again.

@masahi
Copy link
Member

masahi commented Nov 26, 2018

OK, I confirmed locally that test_forward.py is working with CombineParallelConv2D enabled. You can roll back OPT_PASS_LEVEL to 1 if you want. This way we can test CombineParallelConv2D pass on other end to end compilation test cases for free.

@tqchen
Copy link
Member

tqchen commented Nov 26, 2018

@vinx13 Let us set the CombineParallelConv2D to opt_level=3

@tqchen
Copy link
Member

tqchen commented Nov 26, 2018

@vinx13 can you also follow up to check the current status on the inception style models to see the actual benefit of this pass? since we already support inception models

@tqchen
Copy link
Member

tqchen commented Nov 26, 2018

Copy link
Member

@masahi masahi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I thought Inception test was coming. The fix looks good.

@tqchen tqchen merged commit 7880b50 into apache:master Nov 27, 2018
@tqchen
Copy link
Member

tqchen commented Nov 27, 2018

Thanks, @vinx13 ! can you confirm the inception perf and post a followup?

@tqchen
Copy link
Member

tqchen commented Nov 27, 2018

Thanks, @masahi @vinx13 , this is merged

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants