You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/01/13 01:28:06 UTC
[GitHub] [tvm] areusch opened a new pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
areusch opened a new pull request #7266:
URL: https://github.com/apache/tvm/pull/7266
* This change is preparation to support autotuning in microTVM. It
also cleans up a loose end in the microTVM RPC server
implementation.
* Randomness is needed in two places of the CRT:
1. to initialize the Session nonce, which provides a more robust
way to detect reboots and ensure that messages are not confused
across them.
2. to fill input tensors when timing AutoTVM operators (once
AutoTVM support lands in the next PR).
* This change adds TVMPlatformGenerateRandom, a platform function for
generating non-cryptographic random data, to service those needs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561233084
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
My concern is to keep the crt flow upto the standard as zephyr example you are showing here :).
If its not too much trouble, I would think the zephyr implementation is superior (might be a delta difference though) and would like to have it on the crt as well since you have the code already and I think the only difference is usage of rand_r, which is fine for now..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560812251
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
Well, I was actually referring to make it go to common util header and include it where it will be used, thus common impl is explicitly included where its needed -- ( if none uses it then it should not get included, hence not linked ? ). To clarify, this utility function Im refferring here would be inside TVMPlatformGenerateRandom not TVMPlatformGenerateRandom itself.
Anyway, copy-paste stub might be ok for now, but curious why this one has to be different from zephyr one ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r558513482
##########
File path: tests/micro/qemu/zephyr-runtime/src/main.c
##########
@@ -161,6 +162,25 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
return kTvmErrorNoError;
}
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ while (num_bytes > 0) {
Review comment:
rewritten to use for-loop!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561139856
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
ah i see, yeah that would work. hmm, I think if we had the ability to template the random block type (I.e. uint32_t right now) without a C macro i'd do it, but maybe it's okay to leave it as is for now.
the main difference between the zephyr one and the host one is that the underlying rng function is different. the host uses `rand_r` and zephyr uses `sys_rand32_get`. note that this main.cc is not compiled using zephyr but rather just directly against the system headers, and it uses posix pipes to communicate.
##########
File path: tests/micro/qemu/zephyr-runtime/src/main.c
##########
@@ -161,6 +162,27 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
return kTvmErrorNoError;
}
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ uint32_t random; // one unit of random data.
+
+ // Fill parts of `buffer` which are as large as `random`.
+ size_t num_full_blocks = num_bytes / sizeof(random);
+ for (int i = 0; i < num_full_blocks; ++i) {
+ random = sys_rand32_get();
+ memcpy(&buffer[i * sizeof(random)], &random, sizeof(random));
+ }
+
+ // Fill any leftover tail which is smaller than `random.
Review comment:
done
##########
File path: tests/micro/qemu/zephyr-runtime/src/main.c
##########
@@ -161,6 +162,27 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
return kTvmErrorNoError;
}
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ uint32_t random; // one unit of random data.
Review comment:
simplified the tail block computation at the end of mine using modulo operator. thanks for the suggestion!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560563123
##########
File path: tests/micro/qemu/zephyr-runtime/src/main.c
##########
@@ -161,6 +162,27 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
return kTvmErrorNoError;
}
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ uint32_t random; // one unit of random data.
+
+ // Fill parts of `buffer` which are as large as `random`.
+ size_t num_full_blocks = num_bytes / sizeof(random);
+ for (int i = 0; i < num_full_blocks; ++i) {
+ random = sys_rand32_get();
+ memcpy(&buffer[i * sizeof(random)], &random, sizeof(random));
+ }
+
+ // Fill any leftover tail which is smaller than `random.
Review comment:
It's missing a closing single quote closing after ` random`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561221069
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
The thing Im struggling to understand is why throwing away 24 bits is relatively not important compared to the other. Is it because these runtimes are intended to run on platforms/devices with different compute ability ? -- I thought vanilla crt is also intended run on constrained devices.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561233084
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
My concern is to keep the crt flow upto the standard as zephyr example you are showing here :).
If its not too much trouble, I would think the zephyr implementation is superior (might be a delta difference though) and would like to have it on the crt as well since you have the code already and I think the only change is usage of rand_r, which is fine for now..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560359820
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
this is one of those really tricky areas in embedded. you could imagine factoring out the logic for that into a library function, but you would be implicitly relying on:
1. link-time optimization to ensure the common impl gets deleted when not used
2. a function pointer, which may defeat any inlining and might nullify any performance gains you were expecting
3. an assumption that most platforms can generate 32-bit random numbers, from the POV of making the conceptual complexity worth adding
so for "glue-like" logic like this, unfortunately my opinion is that it's best to copy-paste the stub that fits your particular situation. an embedded transpiler would really help cases like this, but that drags in another debate :)
I think given the main difference is a linear speedup in random blob generation on host simulation, and rand_r should not syscall, i'm going to defer any potential change here to the next PR (which enables autotuning and relies on this RNG a lot more).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560812251
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
Well, I was actually referring to make it go to common util header and include it where it will be used, thus common impl is explicitly included where its needed -- ( if none uses it then it should not get included, hence not linked ? ).
Anyway, copy-paste stub might be ok for now, but curious why this one has to be different from to zephyr one ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560564611
##########
File path: tests/micro/qemu/zephyr-runtime/src/main.c
##########
@@ -161,6 +162,27 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
return kTvmErrorNoError;
}
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ uint32_t random; // one unit of random data.
Review comment:
Maybe use modulo operator and remove `num_full_blocks`? Like:
```suggestion
uint32_t random; // one unit of random data.
size_t block_size = sizeof(random);
size_t num_blocks = num_bytes / block_size;
size_t num_tail_bytes = num_bytes % block_size;
// Fill parts of `buffer` which are as large as `random`.
for (int i = 0; i < num_blocks; ++i) {
random = sys_rand32_get();
memcpy(&buffer[i * block_size], &random, block_size);
}
// Fill any leftover tail which is smaller than `random`.
if (num_tail_bytes > 0) {
random = sys_rand32_get();
memcpy(&buffer[num_bytes - num_tail_bytes], &random, num_tail_bytes);
}
```
##########
File path: tests/micro/qemu/zephyr-runtime/src/main.c
##########
@@ -161,6 +162,27 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
return kTvmErrorNoError;
}
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ uint32_t random; // one unit of random data.
+
+ // Fill parts of `buffer` which are as large as `random`.
+ size_t num_full_blocks = num_bytes / sizeof(random);
+ for (int i = 0; i < num_full_blocks; ++i) {
+ random = sys_rand32_get();
+ memcpy(&buffer[i * sizeof(random)], &random, sizeof(random));
+ }
+
+ // Fill any leftover tail which is smaller than `random.
Review comment:
It's missing a closing single quote closing ` random`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561221069
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
The thing Im struggling to understand is why throwing away 24 bits is relatively not important compared to the other. Is it because these runtimes are intended to run on different compute ability ? -- I thought vanilla crt is also intended run on constrained devices.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#issuecomment-763813132
@liangfu please take a look and approve if you're ok with this now
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560563123
##########
File path: tests/micro/qemu/zephyr-runtime/src/main.c
##########
@@ -161,6 +162,27 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
return kTvmErrorNoError;
}
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ uint32_t random; // one unit of random data.
+
+ // Fill parts of `buffer` which are as large as `random`.
+ size_t num_full_blocks = num_bytes / sizeof(random);
+ for (int i = 0; i < num_full_blocks; ++i) {
+ random = sys_rand32_get();
+ memcpy(&buffer[i * sizeof(random)], &random, sizeof(random));
+ }
+
+ // Fill any leftover tail which is smaller than `random.
Review comment:
It's missing a closing single quote after ` random`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561228097
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
ah I see. I am being a little bit lazy here as right now, this mainly serves (in the tvm repo) to support tests/python/unittest/test_crt.py. I can implement a better RNG here if it would help.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen merged pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #7266:
URL: https://github.com/apache/tvm/pull/7266
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] liangfu commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r558228081
##########
File path: tests/micro/qemu/zephyr-runtime/src/main.c
##########
@@ -161,6 +162,25 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
return kTvmErrorNoError;
}
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ while (num_bytes > 0) {
Review comment:
Since the loop should run `num_bytes / sizeof(random)` iterations, I think it's better to use for-loop instead of using while-loop paired with continue statement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#issuecomment-761121252
@liangfu thanks for the review, please take another look when you have a minute
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561221069
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
The thing Im struggling to understand is why throwing away 24 bit is relatively not important compared to the other. Is it because these runtimes are intended to run on different compute ability ? -- I thought vanilla crt is also intended run on constrained devices.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561233084
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
My concern is to keep the crt flow upto the standard as zephyr example you are showing here :).
If its too much trouble, I would think the zephyr implementation is superior (might be a delta difference though).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561233084
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
My concern is to keep the crt flow upto the standard as zephyr example you are showing here :).
If its too much trouble, I would think the zephyr implementation is superior (might be a delta difference though).
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
My concern is to keep the crt flow upto the standard as zephyr example you are showing here :).
If its not too much trouble, I would think the zephyr implementation is superior (might be a delta difference though).
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
My concern is to keep the crt flow upto the standard as zephyr example you are showing here :).
If its not too much trouble, I would think the zephyr implementation is superior (might be a delta difference though) and would like to have it on the crt as well since you have the code already and I think the only change is usage of rand_r, which is fine for now..
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
My concern is to keep the crt flow upto the standard as zephyr example you are showing here :).
If its not too much trouble, I would think the zephyr implementation is superior (might be a delta difference though) and would like to have it on the crt as well since you have the code already and I think the only difference is usage of rand_r, which is fine for now..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561212157
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
well in this case we don't have a tail to worry about because we generate 32-bit random integers but throw away the upper 24 bits, and write the lower 8 bits to successive bytes in `buffer`. we are throwing away randomness but it's pseudorandomness anyhow. I may revisit this if generating random tensors is slow in the AutoTVM PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561233084
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
My concern is to keep the crt flow upto the standard as zephyr example you are showing here :).
If its not too much trouble, I would think the zephyr implementation is superior (might be a delta difference though).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560812251
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
Well, I was actually referring to make it go to common util header and include it where it will be used, thus common impl is explicitly included where its needed -- ( if none uses it then it should not get included, hence not linked ? ).
Anyway, copy-paste stub might be ok for now, but curious why this one has to be different from zephyr one ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen merged pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #7266:
URL: https://github.com/apache/tvm/pull/7266
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r561206556
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
Alright, I think its OK to use the different rng function given the compilation flows. However, I was referring to difference in handling (and absence of it) the tail. Would it be possible to have that kind of code with the different rng here as well ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r559456083
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
I see that zephyr one has a better version -- as in all the 4 bytes of random is being used with a prologue to handle the tail. I think that is a better implementation. Since you have it already, could anything be done to make that part common in the code base and use it here as well ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#issuecomment-760625791
@liangfu @tqchen @tmoreau89 @gromero @tom-gall @u99127 @leandron @manupa-arm
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560359820
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
this is one of those really tricky areas in embedded. you could imagine factoring out the logic for that into a library function, but you would be implicitly relying on:
1. link-time optimization to ensure the common impl gets deleted when not used
2. a function pointer, which may defeat any inlining
3. an assumption that most platforms can generate 32-bit random numbers, from the POV of making the conceptual complexity worth adding
so for "glue-like" logic like this, unfortunately my opinion is that it's best to copy-paste the stub that fits your particular situation. an embedded transpiler would really help cases like this, but that drags in another debate :)
I think given the main difference is a linear speedup in random blob generation on host simulation, and rand_r should not syscall, i'm going to defer any potential change here to the next PR (which enables autotuning and relies on this RNG a lot more).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #7266:
URL: https://github.com/apache/tvm/pull/7266#discussion_r560812251
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
Well, I was actually refferring to make it go to common util header and include it where it will be used, thus common impl is explicitly included where its needed -- ( if none uses it then it should not get included ? ).
Anyway, copy-paste stub might be ok for now, but curious why this one has to be different from to zephyr one ?
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -93,6 +94,20 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
g_utvm_timer_running = 0;
return kTvmErrorNoError;
}
+
+static_assert(RAND_MAX >= (1 << 8), "RAND_MAX is smaller than acceptable");
+unsigned int random_seed = 0;
+tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
+ if (random_seed == 0) {
+ random_seed = (unsigned int)time(NULL);
+ }
+ for (size_t i = 0; i < num_bytes; ++i) {
+ int random = rand_r(&random_seed);
+ buffer[i] = (uint8_t)random;
Review comment:
Well, I was actually referring to make it go to common util header and include it where it will be used, thus common impl is explicitly included where its needed -- ( if none uses it then it should not get included ? ).
Anyway, copy-paste stub might be ok for now, but curious why this one has to be different from to zephyr one ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org