Skip to content

[NFC] Make flags more descriptive#3330

Merged
hanhanW merged 5 commits intoiree-org:mainfrom
hanhanW:flags
Oct 5, 2020
Merged

[NFC] Make flags more descriptive#3330
hanhanW merged 5 commits intoiree-org:mainfrom
hanhanW:flags

Conversation

@hanhanW
Copy link
Copy Markdown
Contributor

@hanhanW hanhanW commented Oct 2, 2020

Fixes #3248

@google-cla google-cla Bot added the cla: yes label Oct 2, 2020
@hanhanW
Copy link
Copy Markdown
Contributor Author

hanhanW commented Oct 2, 2020

We can consider to update flag names in iree-run-mlir. I didn't do it because the style there looks different. Do we want to replace --input-values with --function-inputs for it?

@antiagainst
Copy link
Copy Markdown
Member

antiagainst commented Oct 2, 2020

Could you also update the flags in iree-run-module-vulkan-gui-main.cc and build_apk.sh? They are newly added files. :) It would be nice to perform a grep to make sure there are no others left.

iree-run-mlir uses cli parsing from LLVM and it's following LLVM style (using - instead of _). It would also be nice to unify the words used in options there though. But it can happen as another pull request.

antiagainst
antiagainst requested changes Oct 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@antiagainst antiagainst left a comment

Choose a reason for hiding this comment

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

We also need to update flags in iree-run-module-vulkan-gui-main.cc and build_apk.sh.

@hanhanW
Copy link
Copy Markdown
Contributor Author

hanhanW commented Oct 2, 2020

I updated iree-run-module-vulkan-gui-main.cc and build_apk.sh, thanks for pointing them out!

New finding:

  1. Serialization in tf_test_utils.py, ie, creating flagfile
  2. I think iree-check-module-main.cc has the same context, so also change the variable naming there, although it doesn't have flags.

phoenix-meadowlark
phoenix-meadowlark reviewed Oct 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@phoenix-meadowlark phoenix-meadowlark left a comment

Choose a reason for hiding this comment

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

The modifications to the benchmarking readme and tensorflow integrations LTGM!

@hanhanW hanhanW merged commit 8255d34 into iree-org:main Oct 5, 2020
@hanhanW hanhanW deleted the flags branch October 5, 2020 14:32
hanhanW added a commit to hanhanW/iree that referenced this pull request Oct 5, 2020
@hanhanW
Replace -input-value with -function-value for iree-run-mlir.
b3a16bc 
Follow up on iree-org#3330 to unify words in
options.

Part of iree-org#3248
@hanhanW hanhanW mentioned this pull request Oct 5, 2020
Merged
hanhanW added a commit that referenced this pull request Oct 5, 2020
@hanhanW
Replace -input-value with -function-input for iree-run-mlir. (#3346)
c5de501 
Follow up on #3330 to unify words in
options.

Part of #3248
@GMNGeoffrey GMNGeoffrey mentioned this pull request Oct 5, 2020
Merged
@hanhanW hanhanW mentioned this pull request Oct 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phoenix-meadowlark phoenix-meadowlark phoenix-meadowlark left review comments

@antiagainst antiagainst antiagainst approved these changes

@GMNGeoffrey GMNGeoffrey GMNGeoffrey approved these changes

@benvanik benvanik Awaiting requested review from benvanik

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Confusing -input_file, -inputs, and -inputs_file flags in tools

4 participants

@hanhanW @antiagainst @GMNGeoffrey @phoenix-meadowlark