[PATCH] D62088: [compiler-rt][builtins] Scaled Integer log10()

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

Please add test cases for scale=0 and scale=width as I assume those need special handling (UB right now?).
And if scale=0 and scale=width needs special handling, then I guess scale=1 and scale=width-1 are new boundary values so I maybe it would be nice to have tests for those scales as well.



================
Comment at: compiler-rt/lib/builtins/log10scaleddi3.c:64
+int64_t __log10scaleddi3(uint64_t x, uint32_t scale) {
+  int64_t b = UINT64_C(1) << (scale - 1);
+  int64_t y = 0;
----------------
So with scale=0 being a valid input this results in a negative shift count (UB).


================
Comment at: compiler-rt/lib/builtins/log10scaleddi3.c:66
+  int64_t y = 0;
+  uint64_t oneval = UINT64_C(1) << scale;
+  uint64_t baseval = ((uint64_t)BASE) << scale;
----------------
So with scale=64 being a valid input this results in a too big shift count (UB).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62088/new/

https://reviews.llvm.org/D62088