More tiny fixes #52
More tiny fixes #52
Conversation
ngc92
commented
Jan 4, 2026
ngc92
commented
- rework philox RNG code
- add explicit "empty" tests to Tensor class
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Philox random number generator API and adds convenience methods to the Tensor class for checking empty state. The main changes include replacing multiple RNG generate calls with a single call that produces multiple values at once, and adding an empty() method and explicit operator bool() to Tensor for null-checking.
- Refactored Philox4x32 RNG to support generating multiple random numbers in a single call using std::span
- Added
empty()andexplicit operator bool()methods to Tensor struct for checking null data pointers - Updated all RNG usage sites to use the new consolidated API
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utilities/tensor.h | Adds empty() method and explicit operator bool() for checking if tensor data is null |
| src/utilities/philox.h | Refactors RNG API with new generate_4() and templated generate() methods that accept std::span |
| src/utilities/lazy_allocator.cpp | Uses new Tensor operator bool() to check for null backing in tracing mode |
| src/utilities/comm.h | Changes schedule_destructive_all_to_all parameter from const Tensor& to Tensor& |
| src/utilities/comm.cpp | Updates function signature to match header change |
| src/models/llama_model.cpp | Fixes indentation/whitespace |
| src/models/llama_gradients.cpp | Converts from multiple RNG generate calls to single call with std::span |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto rng_1 = mRng.generate(2*mStepCounter + 0, layer_idx); | ||
| auto rng_2 = mRng.generate(2*mStepCounter + 1, layer_idx + 12345); | ||
| std::array<std::uint32_t, 8> rng; | ||
| mRng.generate(std::span(rng), 2*mStepCounter, layer_idx); |
There was a problem hiding this comment.
The RNG sequence has been changed. The old code generated the second batch of 4 random numbers with y = layer_idx + 12345, but the new code uses y = layer_idx for all batches. This means rng[4], rng[5], and rng[6] will have different values than they had before, potentially breaking deterministic behavior or changing the results. If this change is intentional, it should be documented.
| mRng.generate(std::span(rng), 2*mStepCounter, layer_idx); | |
| mRng.generate(std::span(rng).subspan(0, 4), 2*mStepCounter, layer_idx); | |
| mRng.generate(std::span(rng).subspan(4, 4), 2*mStepCounter, layer_idx + 12345); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ngc92
force-pushed
the
more-tiny-fixes
branch
2 times, most recently
from
4020148 to
9fff7cc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
