-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add packing for int8 1x1 convolution and support the int8 group convolution on X86 #2991
Conversation
cc @yzhliu @yidawang @anijain2305 @ajtulloch |
@llyfacebook Please check the testcase, because of not all test machines support avx512, we need to optionally skip them. |
@yzhliu @kevinthesun can you help follow up and review this PR? |
Thanks, I will skip the tests. |
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.
@llyfacebook would you please help to elaborate the data/kernel layout for each scenario? It's pretty confusing to me so far.
topi/python/topi/nn/conv2d.py
Outdated
raise ValueError("not support this layout {} yet".format(data_layout)) | ||
|
||
|
||
if data_layout == 'NHWC': |
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.
shall we use kernel_layout
instead? as data_layout
might not be necessarily binded to kernal_layout
.
I'm actually a bit confused with the int8 conv layout, for NHWC data, what kernal layout is it?
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.
I think I was mainly following the data layout and kernel layout corresponding relationship here: https://github.com/dmlc/tvm/blob/147ea3b0ca147b527086228d524a2f68f872112d/topi/python/topi/nn/conv2d.py#L284
topi/python/topi/x86/conv2d.py
Outdated
@@ -62,6 +63,9 @@ def _create_tuning_space(cfg, data, kernel, strides, padding, dilation, layout): | |||
if layout == 'NCHW': | |||
n, ic, h, w = dshape | |||
oc, _, kh, kw = kshape | |||
elif layout == 'NHWC': | |||
n, h, w, ic = dshape | |||
oc, _, kh, kw = kshape |
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.
kh, kw, oc, _
?
topi/python/topi/x86/conv2d.py
Outdated
def _declaration_conv(cfg, data, kernel, strides, padding, dilation, layout, out_dtype): | ||
out_dtype = data.dtype if out_dtype is None else out_dtype | ||
padding = padding if isinstance(padding, (tuple, list)) else (padding, padding) | ||
strides = strides if isinstance(strides, (tuple, list)) else (strides, strides) | ||
dilation = dilation if isinstance(dilation, (tuple, list)) else (dilation, dilation) | ||
|
||
_, _, kh, kw = get_const_tuple(kernel.shape) |
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.
so, for data=NHWC & fp32, kernel=HWIO, while for data=NHWC & int8, kernel=OIHW?
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.
Thanks. I will spend some time this week unifying them.
topi/python/topi/x86/conv2d.py
Outdated
conv2d_avx_1x1._schedule_conv_nhwc_pack_int8(*args) | ||
else: | ||
raise ValueError("Only support 1x1 kernel with " | ||
"schedule template.") |
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.
please make the fatal msg more detailed other than just "schedule template"
@llyfacebook please update as per review comments |
@yzhliu please followup on this PR |
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.
otherwise looks good to me.
topi/python/topi/x86/conv2d.py
Outdated
if layout == 'NCHW': | ||
_create_tuning_space(cfg, data, kernel, strides, padding, dilation, layout) | ||
if cfg.is_fallback: | ||
_get_default_config(cfg, data, kernel, strides, padding, out_dtype) | ||
return _declaration_conv_impl(cfg, data, kernel, strides, | ||
padding, dilation, layout, out_dtype) | ||
# KHOI kernel layout is for NHWC and HWCN |
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.
# KHOI kernel layout is for NHWC and HWCN | |
# HWOI kernel layout is for NHWC and HWCN |
Thanks @llyfacebook This is merged |
…lution on X86 (apache#2991) * Support the 1x1 int8 conv with NHWC layout and weight packing fix linter * fix the memoize issue * fix the failed nhwc test * add the schedule for pack to unbreak other tests * skip avx512 compile * Support the 1x1 int8 conv with NHWC layout and weight packing fix linter * fix the memoize issue * fix the failed nhwc test * add the schedule for pack to unbreak other tests * skip avx512 compile * Unify the data_layout and kernel_layout relation * add asf header * fix the comment * retrigger the build/test
…lution on X86 (apache#2991) * Support the 1x1 int8 conv with NHWC layout and weight packing fix linter * fix the memoize issue * fix the failed nhwc test * add the schedule for pack to unbreak other tests * skip avx512 compile * Support the 1x1 int8 conv with NHWC layout and weight packing fix linter * fix the memoize issue * fix the failed nhwc test * add the schedule for pack to unbreak other tests * skip avx512 compile * Unify the data_layout and kernel_layout relation * add asf header * fix the comment * retrigger the build/test
Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers.