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.

Sign up for GitHub

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

Jump to bottom

Fix small typo in nn.conv2d_gemm_weight_transform #5925

Merged
merged 2 commits into from Jun 30, 2020
Merged

Fix small typo in nn.conv2d_gemm_weight_transform #5925

merged 2 commits into from Jun 30, 2020

Conversation

giuseros
Copy link
Contributor

This is fixing a small typo in conv2d_gemm_weight_transform which causes the compilation to fail for AArch64 (NHWC) targets.

Change-Id: I7844d898ebf82592f78f478982262ef95f83cc3e

Fix small typo in nn.conv2d_gemm_weight_transform
0815928 
Change-Id: I7844d898ebf82592f78f478982262ef95f83cc3e
@giuseros
Copy link
Contributor Author

cc @anijain2305 @FrozenGene @u99127

@tqchen
Copy link
Member

tqchen commented Jun 25, 2020

Thanks @giuseros can you add a regression test case as per https://tvm.apache.org/docs/contribute/code_review.html#ensure-test-coverage?

@tqchen tqchen added the status: need test case need test cases to cover the change label Jun 25, 2020
@tqchen
Copy link
Member

tqchen commented Jun 27, 2020

ping @giuseros

@giuseros
Copy link
Contributor Author

Hi @tqchen,
I was off yesterday. Is it ok if I work on this on Monday?
Sorry about that

@tqchen
Copy link
Member

tqchen commented Jun 27, 2020

feel free to take your time

Add TOPI conv2d_gemm unit tests
ef5a75f 
Change-Id: I9ed82a68acffcf0dd9720781f8be4aada9d8e6e4
@giuseros
Copy link
Contributor Author

giuseros commented Jun 29, 2020 •
edited

Hi @tqchen ,
I followed the same approach for the other conv2ds. I added a test_conv2d_nhwc in the test_topi_conv2d_int8.py. This would have caught the typo we are discussing, and I was able also to fix another small typo in conv2d_gemm.py (which had to do with wrong TIR operator precedence).

Please, let me know what do you think.

@giuseros
Copy link
Contributor Author

Hi @tqchen ,
Any update on this?

Thanks a lot

@tqchen
Copy link
Member

tqchen commented Jun 30, 2020

cc @junrushao1994

@tqchen tqchen merged commit 9efe119 into apache:master Jun 30, 2020
@tqchen tqchen added status: accepted and removed status: need test case need test cases to cover the change labels Jun 30, 2020
@tqchen
Copy link
Member

tqchen commented Jun 30, 2020

Thanks @giuseros !

@junrushao
Copy link
Member

Got it. Thanks!

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 30, 2020
Fix small typo in nn.conv2d_gemm_weight_transform (apache#5925)
df1ab23 
* Fix small typo in nn.conv2d_gemm_weight_transform

Change-Id: I7844d898ebf82592f78f478982262ef95f83cc3e

* Add TOPI conv2d_gemm unit tests

Change-Id: I9ed82a68acffcf0dd9720781f8be4aada9d8e6e4
@giuseros giuseros deleted the fix_conv_gemm_aarch64 branch July 1, 2020 13:42
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Jul 2, 2020
@zhiics
Fix small typo in nn.conv2d_gemm_weight_transform (apache#5925)
460c0ec 
* Fix small typo in nn.conv2d_gemm_weight_transform

Change-Id: I7844d898ebf82592f78f478982262ef95f83cc3e

* Add TOPI conv2d_gemm unit tests

Change-Id: I9ed82a68acffcf0dd9720781f8be4aada9d8e6e4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tqchen tqchen tqchen approved these changes

Assignees
No one assigned
Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@giuseros @tqchen @junrushao