Skip to content

Conversation

@nsthorat
Copy link
Contributor

@nsthorat nsthorat commented Oct 8, 2019

  • Each kernel gets its own cc_library and file.
  • A new cc_library encapsulates all of them: ":all_kernels"
  • backend.cc contains global state and backend utilities.
  • Add linkstatic=1 so the compiler yells before emscripten fails at linking (it doesn't support dynamic linking, e.g. extern globals).

This change is Reviewable

@nsthorat nsthorat requested a review from dsmilkov October 8, 2019 17:05
Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 10 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)


tfjs-backend-wasm/src/cc/backend.cc, line 30 at r1 (raw file):

std::map<int, TensorInfo> data;

TensorInfo get_tensor_info(int tensor_id) { return data.at(tensor_id); }

make get_tensor_info() which is a public api live under tfjs/backend namespace , not just tfjs
This way other files that include backend.h can call backend::get_tensor_info which tells you from which file those imports came.
https://google.github.io/styleguide/cppguide.html#Namespaces


tfjs-backend-wasm/src/cc/kernels.h, line 15 at r1 (raw file):

 * ===========================================================================*/

#ifndef TFJS_WASM_KERNELS_H_

make sure each new file has these guards to defend against duplicate code.
https://google.github.io/styleguide/cppguide.html#The__define_Guard

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat)


tfjs-backend-wasm/src/cc/backend.cc, line 28 at r1 (raw file):

// Maps a unique tensor id to info about that tensor. The map owns all of its
// entries.
std::map<int, TensorInfo> data;

move data to unnamed namespace which makes it truly private. see https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables

Copy link
Contributor Author

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov)


tfjs-backend-wasm/src/cc/backend.cc, line 28 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

move data to unnamed namespace which makes it truly private. see https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables

Done


tfjs-backend-wasm/src/cc/backend.cc, line 30 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

make get_tensor_info() which is a public api live under tfjs/backend namespace , not just tfjs
This way other files that include backend.h can call backend::get_tensor_info which tells you from which file those imports came.
https://google.github.io/styleguide/cppguide.html#Namespaces

Done


tfjs-backend-wasm/src/cc/kernels.h, line 15 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

make sure each new file has these guards to defend against duplicate code.
https://google.github.io/styleguide/cppguide.html#The__define_Guard

all new .h files have this already :)

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 10 files at r1, 4 of 4 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@nsthorat nsthorat changed the title Modularize the WASM kernels into separate build targets and files. Modularize the WASM kernels into separate build targets. Oct 8, 2019
@nsthorat nsthorat merged commit 3833990 into master Oct 8, 2019
@nsthorat nsthorat deleted the cpp-mod branch October 8, 2019 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants