Skip to content

Conversation

@shresthamalik
Copy link
Contributor

@shresthamalik shresthamalik commented Jul 23, 2019

  • RewritePass has an additional phase after EnterInCatalog to RemoveNGraphAssigns.
  • EncapsulateOp directly updates the Variable with the computed value
  • Compute Op output tensors are retrieved with the help of Catalog

@shresthamalik shresthamalik added the wip Work in progress label Jul 23, 2019
@shresthamalik shresthamalik requested review from kanvi-nervana and sayantan-nervana and removed request for kanvi-nervana July 24, 2019 21:26
@shresthamalik shresthamalik removed the wip Work in progress label Jul 24, 2019
Copy link
Contributor

@kanvi-nervana kanvi-nervana left a comment

Choose a reason for hiding this comment

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

Minor comments

copy_log_str << " COPY_INP_VAL[0]";
}

// input[1] cannot be from NGraphEncap Op
Copy link
Contributor

Choose a reason for hiding this comment

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

May be give more details here

if (NGraphCatalog::GetCopyToTFFromEncapOutputInfoMap(output_key)) {
if (var->copy_ng_to_tf()) {
number_of_copies++;
copy_log_str << " COPY_TF ";
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe change
copy_log_str << " COPY_TF "; -----> copy_log_str << " COPY_TO_TF ";

Co-Authored-By: kanvi-nervana <kanvi.khanna@intel.com>
@sayantan-nervana sayantan-nervana added the ready to merge This PR is the next in the queue. label Jul 25, 2019
Copy link
Contributor

@sayantan-nervana sayantan-nervana left a comment

Choose a reason for hiding this comment

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

LGTM

OP_REQUIRES_OK(ctx, ctx->resource_manager()->Lookup<NGraphVar>(
ctx->resource_manager()->default_container(),
ref_var_name, &var));
current_ng_tensor = var->ng_tensor();
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self: important bit here.

@shresthamalik shresthamalik merged commit b8944f7 into master Jul 25, 2019
gopoka pushed a commit that referenced this pull request Oct 28, 2019
…sign

Shrestha/integrate remove ng assign
@ashahba ashahba deleted the shrestha/Integrate_RemoveNGAssign branch April 24, 2020 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge This PR is the next in the queue. release candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants