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.

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

Upstream regression: compiler refuses to convert cint to uint32 #469

Closed
mratsim opened this issue Sep 25, 2024 · 1 comment
Closed

Upstream regression: compiler refuses to convert cint to uint32 #469

mratsim opened this issue Sep 25, 2024 · 1 comment
Labels
bug 🪲 Something isn't working upstream 🐉

Comments

@mratsim
Copy link
Owner

mratsim commented Sep 25, 2024

See: https://github.com/mratsim/constantine/actions/runs/11034017591/job/30646539242?pr=468

image

The flow:

Threadpool has a cint numThreads

Threadpool* = ptr object
# All synchronization objects are put in their own cache-line to avoid invalidating
# and reloading cache for immutable fields
barrier{.align: 64.}: SyncBarrier # Barrier for initialization and teardown
# -- align: 64
globalBackoff{.align: 64.}: EventCount # Multi-Producer Multi-Consumer backoff
# -- align: 64
numThreads*{.align: 64.}: cint # N regular workers

It is passed to pseudoRandomPermutation which only accepts an uint32

proc tryStealOne(ctx: var WorkerContext): ptr Task =
## Try to steal a task.
let seed = ctx.rng.nextU32()
for targetId in seed.pseudoRandomPermutation(ctx.threadpool.numThreads):
if targetId == ctx.id:
continue
let stolenTask = ctx.id.steal(ctx.threadpool.workerQueues[targetId])
if not stolenTask.isNil():
return stolenTask
return nil

There should be an error but there aren't, I assume because it's an iterator.

The input is cast to uint32

let maxExclusive = cast[uint32](maxExclusive)
let M = maxExclusive.nextPowerOfTwo_vartime()

And then passed to a function that only accepts unsigned integers with no problem

func nextPowerOfTwo_vartime*(n: SomeUnsignedInt): SomeUnsignedInt {.inline.} =
## Returns x if x is a power of 2
## or the next biggest power of 2
1.SomeUnsignedInt shl (log2_vartime(n-1) + 1)

Forwarded to a dispatcher

func log2_vartime*[T: SomeUnsignedInt](n: T): T {.inline.} =
## Find the log base 2 of an integer
when nimvm:
when sizeof(T) == 8:
T(log2_impl_vartime(uint64(n)))
else:
T(log2_impl_vartime(uint32(n)))
else:
T(log2_c_compiler_vartime(n))

And to the final builtins, depending on Clang/GCC vs ICC/MSVC, the result is a cint that is converted back to T in the previous proc

func log2_c_compiler_vartime*(n: SomeUnsignedInt): cint {.inline.} =
## Compute the log2 of n using compiler builtin
## ⚠ Depending on the compiler:
## - It is undefined if n == 0
## - It is not constant-time as a zero input is checked
if n == 0:
0
else:
when sizeof(n) == 8:
cint(63) - builtin_clzll(n)
else:
cint(31) - builtin_clz(n.uint32)

func log2_c_compiler_vartime*(n: SomeUnsignedInt): cint {.inline.} =
## Compute the log2 of n using compiler builtin
## ⚠ Depending on the compiler:
## - It is undefined if n == 0
## - It is not constant-time as a zero input is checked
when sizeof(n) == 8:
bitscan(bitScanReverse64, n, default = 0)
else:
bitscan(bitScanReverse, c.uint32, default = 0)

Notice the error Error: type mismatch: got 'cint' for 'log2_c_compiler_vartime(n)' but expected 'uint32'

@mratsim mratsim added bug 🪲 Something isn't working upstream 🐉 labels Sep 25, 2024
@mratsim mratsim changed the title Upstream regression: input becomes a cint for an uknown reason Upstream regression: compiler refuses to convert cint to uint32 Sep 25, 2024
@mratsim
Copy link
Owner Author

mratsim commented Sep 25, 2024

Actually already fixed in #459 was working on an unsynced master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working upstream 🐉
Projects
None yet
Development

No branches or pull requests

1 participant