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

TFLM: Enable FVP target in benchmarks #49144

Merged
merged 4 commits into from Jun 1, 2021
Merged

TFLM: Enable FVP target in benchmarks #49144

merged 4 commits into from Jun 1, 2021

Conversation

mansnils
Copy link
Contributor

  • Enable FVP target with and without Ethos-U for person detect benchmarks.
  • Use uint32 instead of int32 in TicksToMs to avoid overflow.
  • Add GetCurrentTicks for FVP target.
  • Use Vela converted model in person detect benchmarks for Ethos-U.

This is fixing: #47071

@mansnils
TFLM: Enable FVP target in benchmarks
aff6f7b 
* Enable FVP target with and without Ethos-U for person detect benchmarks.
* Use uint32 instead of int32 in TicksToMs to avoid overflow.
* Add GetCurrentTicks for FVP target.
* Use Vela converted model in person detect benchmarks for Ethos-U.

Change-Id: Iae7f60fea22c3a652dc39542966b89fd5d541f1e
@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label May 12, 2021
@google-ml-butler
Copy link

Thanks for contributing to TensorFlow Lite Micro.

To keep this process moving along, we'd like to make sure that you have completed the items on this list:

We would like to have a discussion on the Github issue first to determine the best path forward, and then proceed to the PR review.

@google-cla google-cla bot added the cla: yes label May 12, 2021
@mansnils mansnils requested a review from advaitjain May 12, 2021 14:49
@gbaned gbaned self-assigned this May 13, 2021
@gbaned gbaned added comp:micro Related to TensorFlow Lite Microcontrollers prtype:bugfix PR to fix a bug labels May 13, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation May 13, 2021
@mansnils
Copy link
Contributor Author

@advaitjain ping for review

@gbaned gbaned added the awaiting review Pull request awaiting review label May 20, 2021
advaitjain
advaitjain requested changes May 20, 2021
View reviewed changes
Copy link
Member

@advaitjain advaitjain left a comment

Choose a reason for hiding this comment

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

apologies for the delay.

tensorflow/lite/micro/benchmarks/README.md Outdated
@@ -64,3 +64,28 @@ make -f tensorflow/lite/micro/tools/make/Makefile TARGET=sparkfun_edge person_de

Refer to flashing instructions in the [Person Detection Example](https://github.com/tensorflow/tensorflow/blob/master/tensorflow/lite/micro/examples/person_detection/README.md#running-on-sparkfun-edge).


## Run on FVP based on Arm Corstone-300 software.
For more info about the Corstone-300 software see: tensorflow/lite/micro/cortex_m_corstone_300/README.md.
Copy link
Member

Choose a reason for hiding this comment

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

the formatting is a bit odd in markdown.

Can you make all the links clickable (relative paths to the README.md would be fine) and also add in a few newlines to properly demarcate the paragraphs in the rendered markdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes will do

tensorflow/lite/micro/micro_time.h Outdated
inline int32_t TicksToMs(int32_t ticks) {
return static_cast<int32_t>(1000.0f * static_cast<float>(ticks) /
static_cast<float>(ticks_per_second()));
inline uint32_t TicksToMs(uint32_t ticks) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you run into some trouble with int32? I don't think this should be necessary.

The duration calculation

tensorflow/tensorflow/lite/micro/micro_profiler.cc

Line 52 in 996f7d5

int32_t ticks = end_ticks_[i] - start_ticks_[i];

will have the same range whether we have ticks be int32 or uint32. The only requirement is that no more than 2**32 ticks should have passed between two calls to BeginEvent/EndEvent, which I believe has a safe margin.

And there isn't any reason for TicksToMs to return a uint32 since int32 gives us a range of 2147483 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I ran into problems.. need to change from int32_t ticks to uint32_t ticks in PersonDetectionNIerations() in person_detection_benchmark.cc, when running make -j -f tensorflow/lite/micro/tools/make/Makefile TARGET=cortex_m_corstone_300 TARGET_ARCH=cortex-m55 run_person_detection_benchmark - otherwise getting negative results. Then I figure why not change to uint32_t as well in TicksToMs() but you are right TicksToMs() can still use int32_t. I will change back..

Copy link
Member

Choose a reason for hiding this comment

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

ok, I saw the change that you made to change accumulation to uint32, and that makes sense.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes May 20, 2021
advaitjain
advaitjain reviewed May 27, 2021
View reviewed changes
tensorflow/lite/micro/cortex_m_corstone_300/system_setup.cc Outdated
Comment on lines 67 to 74
static bool is_initialized = false;
if (!is_initialized) {
KIN1_UnlockAccessToDWT();
KIN1_InitCycleCounter();
KIN1_ResetCycleCounter();
KIN1_EnableCycleCounter();
is_initialized = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

apologies for missing this earlier. Let's move this to InitializeTarget and remove static bool is_inititalized

Copy link
Member

Choose a reason for hiding this comment

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

I went ahead and pushed a commit with this change.

tensorflow/lite/micro/micro_time.h Outdated
inline int32_t TicksToMs(int32_t ticks) {
return static_cast<int32_t>(1000.0f * static_cast<float>(ticks) /
static_cast<float>(ticks_per_second()));
inline uint32_t TicksToMs(uint32_t ticks) {
Copy link
Member

Choose a reason for hiding this comment

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

ok, I saw the change that you made to change accumulation to uint32, and that makes sense.

advaitjain
advaitjain requested changes May 27, 2021
View reviewed changes
Copy link
Member

@advaitjain advaitjain left a comment

Choose a reason for hiding this comment

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

apologies again for the delay. once small nit and then we're ready to go. I'll make sure this PR is merged before we switch to the stand-alone tflm repo.

@advaitjain advaitjain mentioned this pull request May 27, 2021
Closed
advaitjain added a commit to advaitjain/tensorflow that referenced this pull request May 27, 2021
@advaitjain
Fix duration computation in MicroProfiler.
2cbef88 
Prior to this change, if the BeginEvent occurred at a tick that was
larger than EndEvent, we would incorrectly compute the duration for that
event.

The only requirement that we have is that no more than 2^32-1 ticks
should have passed between a call to BeginEvent and EndEvent

The discussion on tensorflow#49144
alerted us to this bug.

Note that we do not have any unit tests for MicroProfiler at this time.
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer May 28, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 28, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 28, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label May 28, 2021
@advaitjain advaitjain added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 28, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 28, 2021
@gbaned gbaned removed the awaiting review Pull request awaiting review label May 28, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Jun 1, 2021
@advaitjain advaitjain added ready to pull PR ready for merge process kokoro:force-run Tests on submitted change labels Jun 1, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 1, 2021
@copybara-service copybara-service bot merged commit 5db477a into tensorflow:master Jun 1, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@advaitjain advaitjain advaitjain approved these changes

Assignees

@gbaned gbaned

Labels
cla: yes comp:micro Related to TensorFlow Lite Microcontrollers prtype:bugfix PR to fix a bug ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mansnils @advaitjain @kokoro-team @gbaned