Store floats as bits #237
Store floats as bits #237
Conversation
rspirv/dr/build/mod.rs
Outdated
| /// Appends an OpConstant instruction with the given 32-bit integer `value`. | ||
| /// or the module if no block is under construction. | ||
| pub fn constant_u32(&mut self, result_type: spirv::Word, value: u32) -> spirv::Word { | ||
| pub fn constant_int32(&mut self, result_type: spirv::Word, value: u32) -> spirv::Word { |
There was a problem hiding this comment.
Nitpicking: could we also name these constant_32bit and constant_64bit? Same for the other types, so that it's a bit more clear that it's a generic "array of bytes" rather than a (float interpreted as) integer.
That also matches the updated doc-comment on this function (and we could have a similar doc-comment on the dr::Operand enum variant).
There was a problem hiding this comment.
Decided to go with bit32 and bit64 instead to avoid any names starting with numbers.
There was a problem hiding this comment.
Thanks! I didn't expect it to be such an intrusive change, but worth it IMO. Will now have to wait for a maintainer to say the same :)
wackbyte
force-pushed
the
no-more-float
branch
from
83825c0
to
f7204a0
Compare
|
I removed the clippy commit 👍 |
wackbyte
force-pushed
the
no-more-float
branch
2 times, most recently
from
8d9475e
to
4a66893
Compare
|
Sorry about that, just had some trouble with git while trying to rebase. |
| @@ -197,10 +197,10 @@ fn disas_constant(inst: &dr::Instruction, type_tracker: &tracker::TypeTracker) - | |||
| debug_assert_eq!(inst.operands.len(), 1); | |||
| let literal_type = type_tracker.resolve(inst.result_type.unwrap()); | |||
| match inst.operands[0] { | |||
| LiteralInt32(value) => disas_instruction(inst, " ", |_| { | |||
There was a problem hiding this comment.
the operand won't be disassembled correctly in case of a floating point literal type due to the underlying disas_literal_int code which does some type checking. Should be extended to support Float types as well otherwise we print an empty string.
There was a problem hiding this comment.
ah, my mistake. nice catch!
There was a problem hiding this comment.
hmm, i looked into it and it looks like this might require something similar to this module in spirv-tools in order to properly deal with NaNs, infinity, 16-bit floats, etc. i think this is outside the scope of this PR, though, so I left a TODO comment and decided to just rely on the Display impls of f32 and f64 to handle most usages, as i believe this is what the behavior was before anyway.
wackbyte
force-pushed
the
no-more-float
branch
from
c8fa449
to
236e3fa
Compare
|
Okay, finally rebased properly. |
wackbyte
force-pushed
the
no-more-float
branch
from
236e3fa
to
258fb57
Compare
|
Alright, changed it to something else |
Closes #16, should help with #189