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

Add Quantize/Dequantize Partitioning #5940

Merged
merged 11 commits into from
Aug 15, 2020

Conversation

weberlo
Copy link
Contributor

@weberlo weberlo commented Jun 27, 2020

Implements step 1 of the Improvements to Automatic Quantization for Bare-Metal RFC.

The code still needs more thorough documentation, and I'm gonna wait to bake the visitors into C++ until we get some design feedback (either here or on the RFC).

@weberlo
Copy link
Contributor Author

weberlo commented Jul 1, 2020

CC @ZihengJiang

@tqchen tqchen added the status: need update need update based on feedbacks label Aug 10, 2020
@tqchen
Copy link
Member

tqchen commented Aug 10, 2020

@weberlo please rebase to resolve the conflict. @ZihengJiang please help to manage this PR

@weberlo
Copy link
Contributor Author

weberlo commented Aug 11, 2020

@ZihengJiang I've rebased and converted the datatype collector pass into C++, but I won't be prioritizing C++ conversion for the other visitors for the next few weeks. To prevent bit rot, I think we should work towards merging the PR in its current state.

@ZihengJiang
Copy link
Contributor

@weberlo Will this affect the original pipeline when the config partition_conversions is disabled?

@tmoreau89
Copy link
Contributor

Nice work @weberlo, do can we add a partition_conversions=disabled unit test as well?

@weberlo
Copy link
Contributor Author

weberlo commented Aug 11, 2020

@weberlo Will this affect the original pipeline when the config partition_conversions is disabled?

@ZihengJiang It won't affect the original pipeline, since we just return the mod here when partition_conversions is disabled.

Nice work @weberlo, do can we add a partition_conversions=disabled unit test as well?

It's implicitly tested in this helper function. I explicitly set it to disabled to make it clearer in the most recent commit.

@ZihengJiang
Copy link
Contributor

@weberlo I believe we would like a general approach to achieve the partitioning in the future, but I am fine with the PR in the current stage. I will approve it after it pass the tests.

on our way

get clooooooser

clean up (part 1)

clean up (part 2)

clean up (part 3)

clean up (part 4)

clean clean

cleaanaannanaaananaananaananaan

clkjsdflkjlfsjdflkj

revert parser changes

add docs

roll lint

roll lint
@weberlo
Copy link
Contributor Author

weberlo commented Aug 13, 2020

@weberlo I believe we would like a general approach to achieve the partitioning in the future, but I am fine with the PR in the current stage. I will approve it after it pass the tests.

@ZihengJiang I agree the current implementation is suboptimal, and it would be much better to integrate it more closely with the quantization pass itself. I went with the approach in this PR, because you're in the process of making significant changes to quantization, and this approach should be robust to those changes. We should certainly revisit this feature once your changes land though.

Copy link
Contributor

@tmoreau89 tmoreau89 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks for clarifying @weberlo. Can you take a look at the CI error to make sure the PR is ready to be merged?

@weberlo
Copy link
Contributor Author

weberlo commented Aug 15, 2020

@tmoreau89 @ZihengJiang CI's finally passing

@tmoreau89 tmoreau89 merged commit 97779a9 into apache:master Aug 15, 2020
@ZihengJiang
Copy link
Contributor

Merged now! Thanks Logan!

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
* Implement quant/dequant partitioning

on our way

get clooooooser

clean up (part 1)

clean up (part 2)

clean up (part 3)

clean up (part 4)

clean clean

cleaanaannanaaananaananaananaan

clkjsdflkjlfsjdflkj

revert parser changes

add docs

roll lint

roll lint

* add option to toggle fully integral check

* convert dtype collector to C++

* remove need for `with_dtype`

* remove unused imports

* roll lint

* partially address feedback

* roll lint

* upgrade to new parser

* retrigger CI

* roll the dice again
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
* Implement quant/dequant partitioning

on our way

get clooooooser

clean up (part 1)

clean up (part 2)

clean up (part 3)

clean up (part 4)

clean clean

cleaanaannanaaananaananaananaan

clkjsdflkjlfsjdflkj

revert parser changes

add docs

roll lint

roll lint

* add option to toggle fully integral check

* convert dtype collector to C++

* remove need for `with_dtype`

* remove unused imports

* roll lint

* partially address feedback

* roll lint

* upgrade to new parser

* retrigger CI

* roll the dice again
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
* Implement quant/dequant partitioning

on our way

get clooooooser

clean up (part 1)

clean up (part 2)

clean up (part 3)

clean up (part 4)

clean clean

cleaanaannanaaananaananaananaan

clkjsdflkjlfsjdflkj

revert parser changes

add docs

roll lint

roll lint

* add option to toggle fully integral check

* convert dtype collector to C++

* remove need for `with_dtype`

* remove unused imports

* roll lint

* partially address feedback

* roll lint

* upgrade to new parser

* retrigger CI

* roll the dice again
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Sep 2, 2020
* Implement quant/dequant partitioning

on our way

get clooooooser

clean up (part 1)

clean up (part 2)

clean up (part 3)

clean up (part 4)

clean clean

cleaanaannanaaananaananaananaan

clkjsdflkjlfsjdflkj

revert parser changes

add docs

roll lint

roll lint

* add option to toggle fully integral check

* convert dtype collector to C++

* remove need for `with_dtype`

* remove unused imports

* roll lint

* partially address feedback

* roll lint

* upgrade to new parser

* retrigger CI

* roll the dice again
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 3, 2020
* Implement quant/dequant partitioning

on our way

get clooooooser

clean up (part 1)

clean up (part 2)

clean up (part 3)

clean up (part 4)

clean clean

cleaanaannanaaananaananaananaan

clkjsdflkjlfsjdflkj

revert parser changes

add docs

roll lint

roll lint

* add option to toggle fully integral check

* convert dtype collector to C++

* remove need for `with_dtype`

* remove unused imports

* roll lint

* partially address feedback

* roll lint

* upgrade to new parser

* retrigger CI

* roll the dice again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants