| Message ID | 20250305-fix-v_exec_initval_nolibc-v1-1-b87b60e43002@iencinas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: riscv: fix v_exec_initval_nolibc.c | expand |
On Wed, Mar 05, 2025 at 05:39:28PM +0100, Ignacio Encinas wrote: > Vector registers are zero initialized by the kernel. Stop accepting > "all ones" as a clean value. > > Note that this was not working as expected given that > value == 0xff > can be assumed to be always false by the compiler as value's range is > [-128, 127]. Both GCC (-Wtype-limits) and clang > (-Wtautological-constant-out-of-range-compare) warn about this. This check was included because the "dirty" value is an implementation detail that I believe is not strongly defined in the ABI. Since linux does always set this value to zero (currently) we can safely remove this check. Reviewed-by: Charlie Jenkins <charlie@rivosinc.com> Tested-by: Charlie Jenkins <charlie@rivosinc.com> > > Signed-off-by: Ignacio Encinas <ignacio@iencinas.com> > --- > I tried looking why "all ones" was previously deemed a "clean" value but > couldn't find any information. It looks like the kernel always > zero-initializes the vector registers. > > If "all ones" is still acceptable for any reason, my intention is to > spin a v2 changing the types of `value` and `prev_value` to unsigned > char. > --- > tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c b/tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c > index 35c0812e32de0c82a54f84bd52c4272507121e35..b712c4d258a6cb045aa96de4a75299714866f5e6 100644 > --- a/tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c > +++ b/tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c > @@ -6,7 +6,7 @@ > * the values. To further ensure consistency, this file is compiled without > * libc and without auto-vectorization. > * > - * To be "clean" all values must be either all ones or all zeroes. > + * To be "clean" all values must be all zeroes. > */ > > #define __stringify_1(x...) #x > @@ -46,7 +46,7 @@ int main(int argc, char **argv) > : "=r" (value)); \ > if (first) { \ > first = 0; \ > - } else if (value != prev_value || !(value == 0x00 || value == 0xff)) { \ > + } else if (value != prev_value || value != 0x00) { \ > printf("Register " __stringify(register) \ > " values not clean! value: %u\n", value); \ > exit(-1); \ > > --- > base-commit: 03d38806a902b36bf364cae8de6f1183c0a35a67 > change-id: 20250301-fix-v_exec_initval_nolibc-498d976c372d > > Best regards, > -- > Ignacio Encinas <ignacio@iencinas.com> >
On 5/3/25 22:49, Charlie Jenkins wrote: > On Wed, Mar 05, 2025 at 05:39:28PM +0100, Ignacio Encinas wrote: >> Vector registers are zero initialized by the kernel. Stop accepting >> "all ones" as a clean value. >> >> Note that this was not working as expected given that >> value == 0xff >> can be assumed to be always false by the compiler as value's range is >> [-128, 127]. Both GCC (-Wtype-limits) and clang >> (-Wtautological-constant-out-of-range-compare) warn about this. > > This check was included because the "dirty" value is an implementation > detail that I believe is not strongly defined in the ABI. Since linux > does always set this value to zero (currently) we can safely remove this > check. Thanks for the review. Just after sending the patch I noticed it should also remove some code that becomes useless after this change: _prev_value_ and _first_ variables were only needed because two "clean" values were supported. I'll send a v2 tomorrow. I'm guessing keeping your "Reviewed-by" and "Tested-by" is the appropriate thing to do as the changes are very simple. Let me know if that's not the case. Thanks again!
On Thu, Mar 06, 2025 at 07:31:22AM +0100, Ignacio Encinas Rubio wrote: > > > On 5/3/25 22:49, Charlie Jenkins wrote: > > On Wed, Mar 05, 2025 at 05:39:28PM +0100, Ignacio Encinas wrote: > >> Vector registers are zero initialized by the kernel. Stop accepting > >> "all ones" as a clean value. > >> > >> Note that this was not working as expected given that > >> value == 0xff > >> can be assumed to be always false by the compiler as value's range is > >> [-128, 127]. Both GCC (-Wtype-limits) and clang > >> (-Wtautological-constant-out-of-range-compare) warn about this. > > > > This check was included because the "dirty" value is an implementation > > detail that I believe is not strongly defined in the ABI. Since linux > > does always set this value to zero (currently) we can safely remove this > > check. > > Thanks for the review. Just after sending the patch I noticed it should > also remove some code that becomes useless after this change: > _prev_value_ and _first_ variables were only needed because two "clean" > values were supported. > > I'll send a v2 tomorrow. I'm guessing keeping your "Reviewed-by" and > "Tested-by" is the appropriate thing to do as the changes are very > simple. Let me know if that's not the case. > > Thanks again! Yes, those changes seem small so you can keep the tags :) - Charlie
diff --git a/tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c b/tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c index 35c0812e32de0c82a54f84bd52c4272507121e35..b712c4d258a6cb045aa96de4a75299714866f5e6 100644 --- a/tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c +++ b/tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c @@ -6,7 +6,7 @@ * the values. To further ensure consistency, this file is compiled without * libc and without auto-vectorization. * - * To be "clean" all values must be either all ones or all zeroes. + * To be "clean" all values must be all zeroes. */ #define __stringify_1(x...) #x @@ -46,7 +46,7 @@ int main(int argc, char **argv) : "=r" (value)); \ if (first) { \ first = 0; \ - } else if (value != prev_value || !(value == 0x00 || value == 0xff)) { \ + } else if (value != prev_value || value != 0x00) { \ printf("Register " __stringify(register) \ " values not clean! value: %u\n", value); \ exit(-1); \
Vector registers are zero initialized by the kernel. Stop accepting "all ones" as a clean value. Note that this was not working as expected given that value == 0xff can be assumed to be always false by the compiler as value's range is [-128, 127]. Both GCC (-Wtype-limits) and clang (-Wtautological-constant-out-of-range-compare) warn about this. Signed-off-by: Ignacio Encinas <ignacio@iencinas.com> --- I tried looking why "all ones" was previously deemed a "clean" value but couldn't find any information. It looks like the kernel always zero-initializes the vector registers. If "all ones" is still acceptable for any reason, my intention is to spin a v2 changing the types of `value` and `prev_value` to unsigned char. --- tools/testing/selftests/riscv/vector/v_exec_initval_nolibc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: 03d38806a902b36bf364cae8de6f1183c0a35a67 change-id: 20250301-fix-v_exec_initval_nolibc-498d976c372d Best regards,