[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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(_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(_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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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(_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:
us...@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #7266: [µTVM] Add TVMPlatformGenerateRandom, a non-cryptographic random number generator.
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(_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:
us...@infra.apache.org
