Skip to content

Update doc loop op#2337

Merged
gramalingam merged 14 commits intoonnx:masterfrom
gramalingam:update-doc-loop-op
Oct 8, 2019
Merged

Update doc loop op#2337
gramalingam merged 14 commits intoonnx:masterfrom
gramalingam:update-doc-loop-op

Conversation

@gramalingam
Copy link
Copy Markdown
Contributor

Change example in Loop's documentation to clarify things. (Redo of earlier PR #2144 to incorporate various suggestions.)

@gramalingam gramalingam requested a review from a team as a code owner September 19, 2019 03:08
Comment thread docs/Operators.md Outdated
%my_local = Add(%a, %b)
%b_out = Sub(%a, %b)
%my_local = Add(%a, %b_in)
%b_out = Sub(%a, %b_in)
Copy link
Copy Markdown
Collaborator

@wschin wschin Sep 19, 2019

Choose a reason for hiding this comment

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

Suggested change
%b_out = Sub(%a, %b_in)
// The value of b_out is used as the value of b_in in the next iteration. Note that it doesn't affect the value of the b_in passed into this operator.
%b_out = Sub(%a, %b_in)

We don't have assignment operator. To match the C code below, we need some comments to explain it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a few more comments to address the two points you mention.

Comment thread docs/Operators.md Outdated
/* Implicitly-defined code: bind actual parameter values
to formal parameter variables of loop-body */
keepgoing_in = keepgoing_out;
b_in = b_out;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
b_in = b_out;
// The variable b_in is passed into the block by value, so the original value of b_in won't be changed.
b_in = b_out;

Comment thread docs/Operators.md
%user_defined_vals = Add(%b, %b)
return %keepgoing_out, %b_out, %user_defined_vals
%my_local = Add(%a, %b_in)
%b_out = Sub(%a, %b_in) // outgoing value of loop-carried-dependency b
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There is no b anymore. This statement doesn't explicitly say that the last value of b_out in one iteration will be the value of b_in in the next iteration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's what the pseudo-code is for, to show how b_out is copied into b_in that assignment statement. Explanatory comments may be more useful outside, in the description. I will see if I can add any more to clarify things there.

Comment thread docs/Operators.md
%b_out = Sub(%a, %b_in) // outgoing value of loop-carried-dependency b
%keepgoing_out = Greater(%my_local, %b_out) // outgoing loop-termination-condition
%user_defined_val = Add(%b_in, %b_in) // scan-output value to be accumulated
return %keepgoing_out, %b_out, %user_defined_val
Copy link
Copy Markdown
Collaborator

@wschin wschin Sep 20, 2019

Choose a reason for hiding this comment

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

[nit] Given the return … here, it looks like the Loop will terminate at the end of the first iteration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it is more than 1 iteration (in the whole example, you mean?), though it may terminate in 2 or 3 iterations (haven't checked fully). I just reused the existing loop logic, thought it is an artificial one.

@gramalingam gramalingam requested a review from a team as a code owner September 23, 2019 18:54
@wschin wschin added this to the 1.7 milestone Sep 25, 2019
Comment thread docs/Changelog.md Outdated
// my_local = 123; // Can't do this. my_local was defined in the the body
// int t = my_local; // Can't do this. my_local is not accessible here.

// These below values are live-out from the loop and therefore accessible
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[nit]

Suggested change
// These below values are live-out from the loop and therefore accessible
// These below values are bound to the outputs of the loop and therefore accessible

Copy link
Copy Markdown
Collaborator

@wschin wschin left a comment

Choose a reason for hiding this comment

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

Looks much clearer! Thanks.

@gramalingam gramalingam merged commit e560009 into onnx:master Oct 8, 2019
@snnn snnn mentioned this pull request Mar 30, 2020
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
* Clarify example in Loop documentation

* Regenerate op documentation

* generate op documentation

* Address PR review comment

* Address PR feedback

* Update defs.cc

* Update Changelog.md

* Update Operators.md
@gramalingam gramalingam deleted the update-doc-loop-op branch April 14, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants