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

[luci] Introduce Compress weights pass #13521

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[luci] Introduce Compress weights pass #13521

wants to merge 1 commit into from

Conversation

SlavikMIPT
Copy link
Contributor

This commit introduces CopressWeightsPass for Conv2D

ONE-DCO-1.0-Signed-off-by: Vyacheslav Bazhenov [email protected]

@SlavikMIPT SlavikMIPT marked this pull request as draft July 25, 2024 11:22
@SlavikMIPT
Copy link
Contributor Author

SlavikMIPT commented Jul 25, 2024 •
edited
Loading

Current encoded array format:
8b - tree size in bits, 8b - data size in bits, encoded Huffman tree and data bitstream

Using example:

Download mobilenet_v1_1.0_224_quant.tflite

tflite2circle mobilenet_v1_1.0_224_quant.tflite mobilenet_v1_1.0_224_quant.circle
circle2circle --compress_weights_huffman mobilenet_v1_1.0_224_quant.circle mobilenet_v1_1.0_224_quant.compressed.circle

Compression results for mobilenet_v1_1.0_224_quant.circle:
4,275,832 bytes -> 2,971,688 bytes (35% compression)

seanshpark
seanshpark reviewed Jul 28, 2024
View reviewed changes
compiler/luci/export/src/CircleExporterUtils.cpp Outdated
case luci::WeightCompression::NONE:
return circle::WeightCompressionType_NONE;
case luci::WeightCompression::HUFFMAN:
return circle::WeightCompressionType_Huffman;
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you are adding weight compression with Huffman coding.
As you adding this define here, I think compress_weights_huffman instead of general compress_weights, would be better name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanshpark fixed, please take a look

@SlavikMIPT SlavikMIPT force-pushed the luci-compress-weights-pass branch 3 times, most recently from 98fc07b to 685f2b9 Compare July 31, 2024 14:48
@SlavikMIPT SlavikMIPT marked this pull request as ready for review July 31, 2024 15:05
@SlavikMIPT SlavikMIPT changed the title [DRAFT][WIP][luci] Introduce Compress weights pass [DRAFT[luci] Introduce Compress weights pass Jul 31, 2024
@SlavikMIPT SlavikMIPT changed the title [DRAFT[luci] Introduce Compress weights pass [DRAFT][luci] Introduce Compress weights pass Jul 31, 2024
@SlavikMIPT SlavikMIPT force-pushed the luci-compress-weights-pass branch 2 times, most recently from 950a0c0 to 2de0564 Compare July 31, 2024 16:17
seanshpark
seanshpark reviewed Jul 31, 2024
View reviewed changes
compiler/luci/pass/src/CompressWeightsPass.cpp Outdated
auto conv2d = dynamic_cast<luci::CircleConv2D *>(node);
if (not conv2d)
continue;
loco::DataType weights_dtype = loco::must_cast<luci::CircleConst *>(conv2d->filter())->dtype();
Copy link
Contributor

Choose a reason for hiding this comment

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

please split lines. I'd like to have easy readable codes.

Suggested change
loco::DataType weights_dtype = loco::must_cast<luci::CircleConst *>(conv2d->filter())->dtype();
auto filter = loco::must_cast<luci::CircleConst *>(conv2d->filter());
auto weights_dtype = filter->dtype();

seanshpark
seanshpark reviewed Jul 31, 2024
View reviewed changes
compiler/luci/pass/src/helpers/HuffmanEncoder.h Outdated
Comment on lines 143 to 150
arr.push_back(
*(static_cast<const uint8_t *>(static_cast<const void *>(&kTreeSizeInBits)) + i));
Copy link
Contributor

Choose a reason for hiding this comment

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

can you plz split lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanshpark Maybe think about better place for HuffmanEncoder.h/HuffmanDecoder.h library? It will be used in onert-micro and other inferences - so in current configuration luci dependency is required, can we place this on top level as separate dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we place this on top level as separate dependency?

Need to think about this. this maybe the first one, ... and I don't think I will like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with duplicate codes. Sharing code each other will complicate dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/Samsung/ONE/pull/13521/files#r1699149426

seanshpark
seanshpark reviewed Jul 31, 2024
View reviewed changes
res/CircleSchema/0.8/circle_schema.fbs Outdated
@@ -831,6 +837,7 @@ table Conv2DOptions {
dilation_h_factor:int = 1;
// Parameters for Conv2D version 8 or above.
// When set, quantized_bias_type defines the dtype for both bias and accumulator.
weight_compression_type:WeightCompressionType = NONE;
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to upgrade to 0.9 and this requires lots of other changes.

CC @hseok-oh

seanshpark
seanshpark reviewed Jul 31, 2024
View reviewed changes
compiler/luci/pass/src/CompressWeightsPass.cpp Outdated
}
else
{
throw std::runtime_error("Huffman weights compression supports s8 and u8");
Copy link
Contributor

Choose a reason for hiding this comment

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

plz do not throw, just debug info is OK.
we do not want to stop circle2circle with this reason.
this throw should be in the import module.

seanshpark
seanshpark reviewed Jul 31, 2024
View reviewed changes
compiler/luci/lang/include/luci/IR/Nodes/CircleConv2D.h Outdated
@@ -34,7 +34,8 @@ namespace luci
*/
class CircleConv2D final : public FixedArityNode<3, CircleNodeImpl<CircleOpcode::CONV_2D>>,
public CircleNodeMixin<CircleNodeTrait::FusedActFunc>,
public CircleNodeMixin<CircleNodeTrait::Bias>
public CircleNodeMixin<CircleNodeTrait::Bias>,
public CircleNodeMixin<CircleNodeTrait::WeightCompression>
Copy link
Contributor

Choose a reason for hiding this comment

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

Q) why does Conv2D have this attribute? why not the filter Constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Q) why does Conv2D have this attribute? why not the filter Constant?

CircleConst is virtual node - and we need somehow export and import circle - so I suggest to set this attribute when importing op from circle and also set this in CircleConst

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand. Please give more explanation.

@seanshpark
Copy link
Contributor

Compression results for mobilenet_v1_1.0_224_quant.circle:
4,275,952 bytes -> 2,966,952 bytes (1.44 compression rate)

@SlavikMIPT , is the purpose of compression to reduce file size? or is there any other reasons?

@seanshpark
Copy link
Contributor

I don't see any code changes in luci/import. is it OK?

@seanshpark
Copy link
Contributor

I recommend to introduce luci-pass-value-py-test to validate compressed file is OK.
which will require luci-interpreter changes too.

@SlavikMIPT
Copy link
Contributor Author

Compression results for mobilenet_v1_1.0_224_quant.circle:
4,275,952 bytes -> 2,966,952 bytes (1.44 compression rate)

@SlavikMIPT , is the purpose of compression to reduce file size? or is there any other reasons?

There are two purposes: reducing file size(for microcontrollers - this can allow to use models which don't fit in flash memory without compression) and reducing memory bandwidth requirements (which can be a bottleneck for hardware accelerators), Huffman and RLE encodings are relatively computationally cheap. Combining this with compression-aware training or quantization we potentially can achieve higher compression rates

@SlavikMIPT SlavikMIPT force-pushed the luci-compress-weights-pass branch 2 times, most recently from 1e12f94 to eb3bbd8 Compare August 9, 2024 10:44
seanshpark
seanshpark reviewed Sep 1, 2024
View reviewed changes
compiler/luci/pass/src/helpers/HuffmanDecoder.h
@@ -0,0 +1,355 @@
/*
Copy link
Contributor

@seanshpark seanshpark Sep 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

1/ this is header only file. is there any particular reason to make so? why not split implementations to .cpp file?
2/ copy right contains only Samsung. is this file made by you from scratch?
3/ there is no .test.cpp file for this. can you add some?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. ok
  2. yes
  3. ok

@hseok-oh
Copy link
Contributor

hseok-oh commented Sep 4, 2024

@hseok-oh , please share your thoughts about modification of circle schema and compressed data.

Looks good.

@SlavikMIPT SlavikMIPT force-pushed the luci-compress-weights-pass branch 9 times, most recently from af43c81 to a4f7dbf Compare September 12, 2024 13:51
@SlavikMIPT SlavikMIPT force-pushed the luci-compress-weights-pass branch 2 times, most recently from 7792c51 to 526d3d3 Compare September 30, 2024 15:11
@SlavikMIPT
Copy link
Contributor Author

Refactored, but I am not sure that code duplication of Decoder/Encoder is good idea - I would think about extracting it into separate component

[luci] Introduce Compress weights pass
9a80ea4 
This commit introduces CopressWeightsPass for Conv2D

ONE-DCO-1.0-Signed-off-by: Vyacheslav Bazhenov <[email protected]>
@SlavikMIPT SlavikMIPT changed the title [DRAFT][luci] Introduce Compress weights pass [luci] Introduce Compress weights pass Oct 2, 2024
jinevening
jinevening reviewed Oct 15, 2024
View reviewed changes
compiler/luci/export/src/CircleTensorExporter.cpp
Comment on lines +581 to +582
if (lhs->size<loco::DataType::FLOAT32>() != rhs->size<loco::DataType::FLOAT32>())
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This code looks very ugly. Isn't it possible to just check lhs->compression() == rhs->compression()?

jinevening
jinevening reviewed Oct 15, 2024
View reviewed changes
compiler/luci/pass/include/luci/Pass/CompressWeightsPass.h
*
* To see the target Op pattern, please visit implementation.
*/
struct CompressWeightsPass final : public logo::Pass
Copy link
Contributor

Choose a reason for hiding this comment

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

The name looks too general. Can you rename it to something like CompressWeightsHuffmanPass?

@jinevening
Copy link
Contributor

It seems that test codes are missing as @seanshpark pointed out.

I left some comments because @SlavikMIPT requested review. But I don't know details about the algorithm. It would be better to add another reviewer, e.g., @hseok-oh.

@seanshpark
Copy link
Contributor

plz split each compiler/* module changes to separate PR.
and this PR change has too many files to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinevening jinevening jinevening left review comments

@Torrero Torrero Awaiting requested review from Torrero

@stamalakhov stamalakhov Awaiting requested review from stamalakhov

@seanshpark seanshpark Awaiting requested review from seanshpark

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SlavikMIPT @seanshpark @hseok-oh @jinevening