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

Upgrade TensorFlow to eaacee173897b77cdb6afd22d5e78154177a10f3 #363

Merged
merged 4 commits into from
Jun 8, 2020
Merged

Upgrade TensorFlow to eaacee173897b77cdb6afd22d5e78154177a10f3 #363

merged 4 commits into from
Jun 8, 2020

Conversation

lgeiger
Copy link
Member

@lgeiger lgeiger commented May 13, 2020

What do these changes do?

This PR upgrades the TensorFlow dependency in order to include eaacee173897b77cdb6afd22d5e78154177a10f3.

Ruy heavily changed their internal API which requires quite a few code changes. A full diff of the changes in ruy can be seen here. This is a first stab at trying to adapt our kernels to the API changes.

How Has This Been Tested?

CI

Related issue number

This is a required update to enable easy int8 benchmarking in #357

@lgeiger lgeiger added the internal-improvement Internal Improvements and Maintenance label May 13, 2020
@lgeiger lgeiger requested a review from a team May 13, 2020 18:19
lgeiger
lgeiger commented May 13, 2020
View reviewed changes
larq_compute_engine/core/bgemm_impl_ruy.h
Comment on lines -107 to +112
transposed_lhs, rhs, mul_params, ruy_context, &dst, bgemm_runtime_path,
&binary_trmul_params);

// pre-pack the lhs and rhs matrices
ruy::PrepackedMatrix prepacked_lhs;
ruy::PrepackedMatrix prepacked_rhs;
ruy::SidePair<ruy::PrepackedMatrix*> prepacked(&prepacked_lhs,
&prepacked_rhs);

const ruy::SidePair<int> origin{0, 0};
const ruy::SidePair<int> rounded_dims{
binary_trmul_params.packed[ruy::Side::kLhs].layout.cols,
binary_trmul_params.packed[ruy::Side::kRhs].layout.cols};

ruy::Tuning tuning = ContextInternal::GetMainThreadTuning(ruy_context);
for (ruy::Side side : {ruy::Side::kLhs, ruy::Side::kRhs}) {
if (prepacked[side]) {
prepacked[side]->data_size = DataSize(binary_trmul_params.packed[side]);
prepacked[side]->sums_size = SumsSize(binary_trmul_params.packed[side]);
prepacked[side]->data = alloc_fn(prepacked[side]->data_size);
prepacked[side]->sums = alloc_fn(prepacked[side]->sums_size);
binary_trmul_params.packed[side].data = prepacked[side]->data;
binary_trmul_params.packed[side].sums = prepacked[side]->sums;
binary_trmul_params.RunPack(side, tuning, origin[side],
rounded_dims[side]);
binary_trmul_params.is_prepacked[side] = true;
}
}
transposed_lhs, internal_rhs, mul_params, ruy_ctx, &internal_dst,
bgemm_runtime_path, &binary_trmul_params);

ruy::TrMul(&binary_trmul_params, ruy_context);
HandlePrepackedCaching(&binary_trmul_params, ruy_ctx);
ruy::TrMul(&binary_trmul_params, ruy_ctx);
Copy link
Member Author

Choose a reason for hiding this comment

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

⚠️ Review with care. I am not familiar with this code so I am not sure what these changes do, but for now at least they seem to compile.

/cc @Tombana @arashb

Copy link
Contributor

Choose a reason for hiding this comment

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

here I was using the RUY's advanced API but it is completely removed in this commit. I need to dedicate more time to figure out how to replicate that.

@lgeiger lgeiger marked this pull request as draft May 13, 2020 18:23
@lgeiger lgeiger force-pushed the tf-nightly-int8-qat-upgrade branch from f96d7c7 to 8b624bb Compare May 14, 2020 17:12
@lgeiger lgeiger changed the base branch from tf-nightly to master May 14, 2020 17:12
@lgeiger lgeiger added the help wanted Extra attention is needed label May 14, 2020
@lgeiger lgeiger force-pushed the tf-nightly-int8-qat-upgrade branch from 8b624bb to fef1bc8 Compare May 19, 2020 14:11
@lgeiger lgeiger mentioned this pull request May 19, 2020
Merged
@AdamHillier AdamHillier added blocked Relies on something else being done first and removed blocked Relies on something else being done first labels Jun 5, 2020
@Tombana
Copy link
Collaborator

Tombana commented Jun 8, 2020

The is_mutable_ error turned out to be an issue with constness of the lhs and rhs matrices. In the tflite/ruy code, a non-const Matrix object was created, and then passed to the ruy::Mul function as a const Matrix. However, since we don't have that function call, our matrix was non-const. This is now fixed.

Then a second error got thrown, because RUY removed the kReference path (now there's only kStandardCpp). For legacy reasons, they set kReference = kStandardCpp. We therefore can simply remove the check for not doing the reference path.

@lgeiger lgeiger marked this pull request as ready for review June 8, 2020 10:22
@lgeiger lgeiger removed the help wanted Extra attention is needed label Jun 8, 2020
@lgeiger lgeiger requested a review from arashb June 8, 2020 10:22
@Tombana Tombana force-pushed the tf-nightly-int8-qat-upgrade branch from 38a0226 to 1562a38 Compare June 8, 2020 10:26
@Tombana
Copy link
Collaborator

Tombana commented Jun 8, 2020

Rebased on master.

@Tombana
Copy link
Collaborator

Tombana commented Jun 8, 2020

Just benchmarked this branch on the pixel. Quicknet got 18.0 ms so there is no performance degradation (just to confirm we are not messing up the ruy prepacking or caching or anything). From my side it can be merged.

@lgeiger
Copy link
Member Author

lgeiger commented Jun 8, 2020

Just benchmarked this branch on the pixel. Quicknet got 18.0 ms so there is no performance degradation (just to confirm we are not messing up the ruy prepacking or caching or anything). From my side it can be merged.

Thanks for integrating all the changes and fixing the PR

arashb
arashb approved these changes Jun 8, 2020
View reviewed changes
Copy link
Contributor

@arashb arashb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Tombana Tombana merged commit f374319 into master Jun 8, 2020
@Tombana Tombana deleted the tf-nightly-int8-qat-upgrade branch June 8, 2020 15:15
lgeiger added a commit that referenced this pull request Jun 8, 2020
@lgeiger
Remove broken include
dbcba3f 
This fixes build problems after merging #363
lgeiger added a commit that referenced this pull request Jun 9, 2020
@lgeiger
Remove broken include
2f6d563 
This fixes build problems after merging #363
Tombana pushed a commit that referenced this pull request Apr 6, 2021
@CNugteren
Move Ikva weight processing to flatbuffer (#363)
5dcbb98 
* Split weight-writer function into a process and interleave function
* Move ProcessWeights function to its own file and make it layer-agnostic
* Add extra bconv test with different filter count
* Fix minor bug in output-transform MLIR pass
* Move weight-processing to an MLIR prepare pass
* Add process-weights step to conv unittest
* Add new flip-weights function for the new filter layout
* Bump Ikva tflite version number
* Update Ikva model example to v0.3 with the latest converter
* Add utility to re-generate model_data_example.cc if needed
* Update lce-utils requirement for a new compute-engine release today
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tombana Tombana Tombana approved these changes

@arashb arashb arashb approved these changes

Assignees
No one assigned
Labels
internal-improvement Internal Improvements and Maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lgeiger @Tombana @arashb @AdamHillier