fix(cuda-macros): correct gpu_printf default float and unsigned formatting - #1353
Open
YeonwooSung wants to merge 3 commits into
Open
YeonwooSung wants to merge 3 commits into
YeonwooSung wants to merge 3 commits into
Conversation
`{:.N}` has no type character, so the default arm packed the argument
as i64 and emitted %.Nlld. C precision on an integer conversion is a
minimum digit count, which turned 3.14159 into 03 instead of 3.14.
Signed-off-by: YeonwooSung <neos960518@gmail.com>
Default `{}` packed every argument as i64 and emitted %lld, so a u64
above i64::MAX printed as a negative number. Visible unsigned arguments
now cast to u64 and use %llu. Bindings whose type the macro cannot see
go through GpuPrintfArg, which also keeps floats off the integer path.
Signed-off-by: YeonwooSung <neos960518@gmail.com>
…printf-default-format Signed-off-by: YeonwooSung <neos960518@gmail.com> # Conflicts: # cuda-oxide/crates/cuda-macros/src/tests/printf.rs
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
gpu_printf!("{:.2}", 3.14159)is documented to print3.14, but a placeholder with precision and no type character leftformat_type == None. That arm packed the argument asi64and emitted%.2lld.3.14159 as i64is3, and C precision on an integer conversion is a minimum digit count, so the result was03, not3.14.The same default path cast every untyped or unsigned value with
as i64and printed it with%lld. Au64abovei64::MAXtherefore wrapped and printed as a negative number.This fixes an unfiled bug (no issue number).
Test that failed first
tests::printf::precision_without_type_on_float_uses_float_conversionfailed before any production change. Expanding{:.2}on3.14159f32produced(3.14159f32) as i64andb"%.2lld\0".After that fix was in,
tests::printf::default_format_on_u64_uses_unsigned_conversionfailed on{}of18446744073709551615u64, which expanded to(18446744073709551615u64) as i64andb"%lld\0".Fix
{:.N}with no type character uses%fandas f64. That includes arguments whose type is not visible in the tokens, so a float is not truncated to an integer.42u64,value as u64) is packed withas u64and printed with%llu.GpuPrintfArg. The format character comes from that trait (%lluforu64,%ffor floats,%d/%lldfor signed integers) instead of always guessingi64.{:x},{:e},{:f},{:08}, and width on an unsuffixed integer.How I verified
as i64/%.2lldandas i64/%lldrespectively.cargo test -p cuda-macros --lib(114 passed), including both new tests.cargo test -p cuda-device --lib default_format_typechecks_for_visible_and_untyped_argstypechecks the expandedgpu_printf!for{:.2}onf32,{}onu64::MAX,{:08},{:x}, and{:e}.cargo test -p cuda-macros(integration tests included) also buildscuda-bindings, which requires a CUDA 13 toolkit. No toolkit is installed in this environment (CUDA_HOME/CUDA_TOOLKIT_PATHunset, no/usr/local/cuda), so those dev-dependency tests were not run. The lib tests do not need the toolkit.