Skip to content

Conversation

@vinx13
Copy link
Member

@vinx13 vinx13 commented Oct 24, 2018

Currently dilate is a separate operation and one has to call dilate + conv2d to compose dilated convolution.
The problems:

  • Sometimes it is better to combine both into a single operator because we may want to write the expresion of dilated convolution directly instread of constructing a dilated kernel and then call compute_inline.
  • The workload in AutoTVM does not reflect dilation. For example, we will use the same configuration for 5x5 kernel and 3x3 kernel with dilation=2 because they have the same workload.

This PR adds dilation as a new argument to conv2d / depthwise_conv2d. If dilation is used, it will call nn.dilate internally in conv2d (like nn.pad). The workload is also updated. This will invalidate AutoTVM logs generated previously.

cc @merrymercy @tqchen @masahi @nishi-t @anijain2305 @Huyuwei

@merrymercy
Copy link
Member

Currently, the log format of autotvm depends on arguments. So every time we change arguments, we will invalidate all old log records. Should we use another design? @tqchen

Since this PR will break old logs, you should provide a log converter for other users.
I can test you converter and update the logs in TopHub.

@vinx13 vinx13 force-pushed the feature/dilated_conv2d branch 2 times, most recently from 6b5c6d7 to 9178e2d Compare October 24, 2018 11:14
@tqchen
Copy link
Member

tqchen commented Oct 24, 2018

I think in this case, adding dilation makes sense, because this makes things consistent with the high level op. We do need to provide a quick upgrade path though, please try make API consistent with high level op https://docs.tvm.ai/langref/relay_op.html#level-2-definitions

@vinx13
Copy link
Member Author

vinx13 commented Oct 25, 2018

@merrymercy Are there cases where dilation is used in tophub logs? To convert old logs to new workload, I will add a dilation field to all old logs. Since we don't have dilation previously, if dilation is used we will have dilated kernel shape in the workload.

@vinx13 vinx13 changed the title [WIP][TOPI] Add dilation argument to conv2d and depthwise_conv2d [TOPI] Add dilation argument to conv2d and depthwise_conv2d Oct 25, 2018
@merrymercy
Copy link
Member

No dilation is used in tophub.

@vinx13
Copy link
Member Author

vinx13 commented Oct 26, 2018

here is the converter for old logs
https://gist.github.com/vinx13/6c9231843a5833f31de7eab938b345b1

@vinx13 vinx13 force-pushed the feature/dilated_conv2d branch 2 times, most recently from 0e94944 to 981289f Compare October 26, 2018 11:12
@tqchen
Copy link
Member

tqchen commented Oct 27, 2018

@vinx13 please check the CI errors, then ask for review again

@vinx13
Copy link
Member Author

vinx13 commented Oct 27, 2018

the test of dilated conv2d nhwc is incorrect, see https://github.com/dmlc/tvm/blob/42dc24a310170577f929f648f477ca2567c8bc9a/topi/tests/python/test_topi_conv2d_nhwc.py#L16
https://github.com/dmlc/tvm/blob/42dc24a310170577f929f648f477ca2567c8bc9a/topi/python/topi/nn/conv2d.py#L277
the dilation should be [dilation_h, dilation_w, 1, 1]

@tqchen How can I update pickle-memorized reference data?

@merrymercy
Copy link
Member

Currently x86 backend in under refactor #1993 .
I think it is better to merge that first. So you can stop all your changes on topi/x86/*.

To update a pickle-memorized data, a trick is to rename the string.
https://github.com/dmlc/tvm/blob/42dc24a310170577f929f648f477ca2567c8bc9a/topi/tests/python/test_topi_conv2d_nhwc.py#L23

@vinx13 vinx13 force-pushed the feature/dilated_conv2d branch 2 times, most recently from c4bc34d to 924e25f Compare October 30, 2018 03:41
# placeholder
Input = tvm.placeholder((batch, in_height, in_width, in_channel), name='Input')
Filter = tvm.placeholder((filter_height, filter_width,filter_channel, channel_multiplier), name='Filter')
DilatedFilter = topi.nn.dilate(Filter, (1, 1, dilation, dilation), name='DilatedFilter')
Copy link
Member Author

Choose a reason for hiding this comment

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

@merrymercy previous dilation is incorrect, I have updated to [dilation, dilation, 1, 1] but current fallback schedule generated invalid ptx on cuda

Copy link
Member

Choose a reason for hiding this comment

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

The filter is incorrect. The dilation is correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

nchw and nhwc have different filter layouts, the filter here is consistent with nn.depthwise_conv2d_nhwc.
https://github.com/dmlc/tvm/blob/b840e9602e357c50124d0c7fb131c52321062570/topi/python/topi/nn/depthwise_conv2d.py#L80

Copy link
Member

Choose a reason for hiding this comment

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

Sorry.
NHWC layout doesn't use autotvm template. I think we can just disable this test, since no one uses this layout.. or you can try to fix the manual schedule https://github.com/dmlc/tvm/blob/b840e9602e357c50124d0c7fb131c52321062570/topi/python/topi/cuda/depthwise_conv2d.py#L119

@vinx13
Copy link
Member Author

vinx13 commented Oct 30, 2018

@merrymercy I have looked into the error in http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/PR-1970/21/pipeline/232#step-255-log-1230. There is one failing case of nchw conv2d on nvptx because tophub logs are invalidated in this pr. The fallback schedule from fallback_with_reference_log produced wrong conv2d result.

@merrymercy
Copy link
Member

There are some errors with nvptx backend. We found some cases before.

The solution can be

  1. Update llvm in CI to 6.0
  2. Disable that test case (for nvptx)

@vinx13
Copy link
Member Author

vinx13 commented Oct 30, 2018

There are some errors with nvptx backend. We found some cases before.

The solution can be

  1. Update llvm in CI to 6.0
  2. Disable that test case (for nvptx)

I tried llvm-6.0 locally but the error still occurs. This error can be fixed by updating tophub logs with dilation arg, or disabling fallback schedule generated from reference log

@merrymercy
Copy link
Member

merrymercy commented Oct 30, 2018

OK. We will first merge #2034, and then merge this PR.

In the meanwhile, you can do update to tophub

  1. Use your converter to convert all latest logs under https://github.com/uwsampl/tvm-distro/tree/master/tophub
    • arm_cpu_v0.03 -> arm_cpu_v0.04, cuda_v0.03 -> cuda_v0.04, opencl_v0.01 -> opencl_v0.02, ..., the only special case is vta_v0.01 -> vta_v0.04
    • Note there are also dense layer workloads. So your convert should only convert conv layers.
    • Then I will merge your PR
  2. Update tophub versions for all backends in tvm

@vinx13 vinx13 force-pushed the feature/dilated_conv2d branch 3 times, most recently from 8724ce6 to f5c0549 Compare October 31, 2018 03:20
@merrymercy
Copy link
Member

Can you also help to update the style of cuda winograd and cuda int8 according to #2034 ?
So we can delete all strange logic in things like _conv2d_arg_to_workloads

@vinx13 vinx13 force-pushed the feature/dilated_conv2d branch from f5c0549 to 9443ecf Compare October 31, 2018 06:11
@tqchen tqchen merged commit 2005f85 into apache:master Oct 31, 2018
@tqchen
Copy link
Member

tqchen commented Oct 31, 2018

Thanks @merrymercy @vinx13 this is now merged

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