Support more uint literals, simplify registering types for GPU, regular finite field pow typo fix
#583
Conversation
Instead of having calls to `maybeAddType` all over the place, we now pass the GPU context into the `nimToGpuType` function and call the helper to register types in there. That makes the most sense and is the cleanest. Initially I was against this, because it was nice that ~nimToGpuType~ was a pure function without dependence on any kind of context state. But the upside is worth it and we don't really need the function in another context anyway.
Summary of ChangesHello @Vindaar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily enhances the GPU compiler's type handling by centralizing the registration of types during the Nim to GPU AST transformation, making the process more robust and easier to maintain. Additionally, it expands the compiler's capability to correctly interpret various unsigned integer literals. A minor, yet important, typo in a finite field exponentiation function has also been addressed, improving the correctness of mathematical operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a few valuable changes. It fixes a critical bug in the pow function for finite fields, where the exponentiation was performed on the input argument instead of the result variable. It also extends support for more unsigned integer literal types in the GPU compiler. Finally, it refactors the type registration logic in the Nim to GPU AST transformation, centralizing it into nimToGpuType for better maintainability. The refactoring is well-executed. I have one suggestion to reduce code duplication in the handling of new unsigned integer literals.
| of nnkUIntLit, nnkUint64Lit: | ||
| result = GpuAst(kind: gpuLit) | ||
| result.lValue = $node.intVal | ||
| result.lType = initGpuType(gtUInt32) | ||
| of nnkUIntLit: | ||
| result.lType = initGpuType(gtUInt64) ## XXX: base on target platform! | ||
| of nnkUInt16Lit: | ||
| result = GpuAst(kind: gpuLit) | ||
| result.lValue = $node.intVal | ||
| result.lType = initGpuType(gtUInt64) ## XXX: base on target platform! | ||
| result.lType = initGpuType(gtUInt16) ## XXX: base on target platform! | ||
| of nnkUInt32Lit: | ||
| result = GpuAst(kind: gpuLit) | ||
| result.lValue = $node.intVal | ||
| result.lType = initGpuType(gtUInt32) ## XXX: base on target platform! |
There was a problem hiding this comment.
These blocks for handling unsigned integer literals are very similar. You can reduce code duplication by using a template or a helper procedure. For example, you could define a template inside toGpuAst:
template uintLit(typ: GpuTypeKind) =
result = GpuAst(kind: gpuLit)
result.lValue = $node.intVal
result.lType = initGpuType(typ) ## XXX: base on target platform!And then use it like this:
of nnkUIntLit, nnkUint64Lit:
uintLit(gtUInt64)
of nnkUInt16Lit:
uintLit(gtUInt16)
of nnkUInt32Lit:
uintLit(gtUInt32)This would make the code more concise and easier to maintain.
There was a problem hiding this comment.
Yeah, I know I could bot. :)
mratsim
left a comment
mratsim
left a comment
There was a problem hiding this comment.
LGTM apart from the default value which should be 32-bit for int, uint float I think.
| ## ``exponent``: a finite field element or big integer | ||
| r = a | ||
| a.pow(exponent) | ||
| r.pow(exponent) |
| result.lValue = $node.intVal | ||
| result.lType = initGpuType(gtUInt32) | ||
| of nnkUIntLit: | ||
| result.lType = initGpuType(gtUInt64) ## XXX: base on target platform! |
There was a problem hiding this comment.
uint should probably be mapped to uint32 by default as well on GPU.
| result.lType = initGpuType(gtUInt32) ## XXX: base on target platform! | ||
| of nnkFloat64Lit, nnkFloatLit: | ||
| result = GpuAst(kind: gpuLit) | ||
| result.lValue = $node.floatVal & "f" |
There was a problem hiding this comment.
iirc, f is float32, and default float in C/Metal/Cuda is float32 not float64 iirc
|
While I agree in principle about the type mappings, in practice I'm starting to consider the opposite approach. Because the code is part of a typed macro and we want the Nim compile time guarantees, if we map int, uint, float to different types than in regular Nim, we deal with many very annoying type mismatch bugs on the Nim side. In the end one anyways has to to all sorts of manual type conversions. Say if int is int32 then it should be fine to add an int to an int32 variable, or assign int to int32. but of course that's not valid Nim. So I'm considering to keep Nim semantics (which this PR of course doesn't do yet) and document that. |
|
Let's merge it in like that for now then. I do think it would be quite verbose to always read u32 / f32 in GPU code but:
|
Decided to put a small typo fix of in-place
powfor finite fields into this PR, because I noticed it in the same context and it's one character change.On the GPU compiler side this:
nimToGpuType. We needed to pass the GPU context into this proc, which initially I didn't intend to do. But the code was getting a bit ugly withmaybeAddTypecalls all over the place.