Skip to content

Add basic SIMD support - #523

Merged
dhardy merged 6 commits into
rust-random:masterfrom
pitdicker:simd_support_basic
Jun 29, 2018
Merged

Add basic SIMD support#523
dhardy merged 6 commits into
rust-random:masterfrom
pitdicker:simd_support_basic

Conversation

@pitdicker

@pitdickerpitdicker commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

This is a rebased version of the branch from #377 (comment).

@TheIronBorn Do you want to review?

TheIronBorn
TheIronBorn reviewed Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs Outdated
// those are usually more random.
let float_size = mem::size_of::<$f_scalar>() * 8;
let precision = $fraction_bits + 1;
let scale = $ty::splat(1.0 / ((1 as $u_scalar << precision) as $f_scalar));

@TheIronBornTheIronBornJun 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: most arithmetic and bitwise operations allow you to just use scalar values:

let scale = $ty::splat(1.0 / ((1as $u_scalar << precision)as $f_scalar));
scale * value
// is the same as
let scale = 1.0 / ((1as $u_scalar << precision)as $f_scalar);
scale * value

TheIronBorn
TheIronBorn reviewed Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs Outdated

let value: $uty = rng.gen();
let fraction = value >> (float_size - $fraction_bits);
fraction.into_float_with_exponent(0) - $ty::splat(1.0 - EPSILON / 2.0)

@TheIronBornTheIronBornJun 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This would be the same:

fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)

@TheIronBorn

TheIronBorn commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

spelling https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L502

@TheIronBorn

TheIronBorn commented Jun 21, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Bug: https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L600

assert!(low < high,"Uniform::new called with `low >= high`");

PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.

This will be fine on most inputs, but for inputs like new(f32x2::new(0.0, 100.0), f32x2::new(100.0, 0.0)) this assert will pass. We should probably have tests for these sort of cases.

The assertions need to be of the form low.lt(high).all()

@pitdicker

pitdicker commented Jun 22, 2018

Copy link
Copy Markdown
ContributorAuthor

PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.

O wow, that is something easy to get wrong! Thank you for reviewing.

I have added a math_helpers module, with:

  • The WideningMultiply implementation from Uniform;
  • A trait for casting integers to floats. Normal floats need to use as, From is not implemented for the integer sizes we use. Vectors don't have as, and need From...
  • A trait to have natural comparison for both floats, and vectors. I only implemented the few things we need.

@pitdicker

pitdicker commented Jun 22, 2018

Copy link
Copy Markdown
ContributorAuthor

The CI fails because of rust-lang/rust#51699.

dhardy
dhardy reviewed Jun 25, 2018
View reviewed changes

@dhardydhardy left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just some nits; meant to post this a couple of days ago

Comment threadsrc/distributions/mod.rs Outdated
mod integer;
#[cfg(feature="std")]
mod log_gamma;
mod math_helpers;

@dhardydhardyJun 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any more mathematical than other parts of the library? You could just call it num_utils (i.e. Number Theory, though arguably that still covers most of the library) or arithmetic.

Comment threadsrc/distributions/math_helpers.rs Outdated

/// `PartialOrd` for vectors compares lexicographically. We want natural order.
/// Only the comparison functions we need are implemented.
pub trait NaturalCompare {

@dhardydhardyJun 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is natural depends on the context which is how PartialOrd confused you. How about SimultaneousOrd?

@dhardy

dhardy commented Jun 26, 2018

Copy link
Copy Markdown
Member

@TheIronBorn do you think this is ready?

I'd like to go over it in more detail myself still, but I think it's pretty neat that we can do this (it's well beyond what most random libs can offer)!

@TheIronBorn

TheIronBorn commented Jun 26, 2018

Copy link
Copy Markdown
Contributor

Looks good! I'll make an SIMD swap_bytes pull request once this merges

dhardy
dhardy approved these changes Jun 27, 2018
View reviewed changes

@dhardydhardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other than those two things looks good.

BTW @TheIronBorn you can make a commit on top of @pitdicker's branch and push to your own branch if you have a fix, but best coordinate between yourselves.

Comment threadsrc/distributions/integer.rs
unsafe {
let ptr = &mut vec;
let b_ptr = &mut *(ptr as *mut $ty as *mut [u8; $bits/8]);
rng.fill_bytes(b_ptr);

@dhardydhardyJun 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look portable to me. Elsewhere we've made an effort to keep things portable; I don't think this needs to be an exception?

Unfortunately it doesn't look like the SIMD types support to_le. @TheIronBorn is this what you mean about using swap_bytes?

@TheIronBornTheIronBornJun 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that’s exactly where we’d use it

Comment threadsrc/distributions/integer.rs
macro_rules! simd_impl {
($bits:expr,) => {};
($bits:expr, $ty:ty, $($ty_more:ty,)*) => {
simd_impl!($bits, $($ty_more,)*);

@dhardydhardyJun 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat usage of recursive macros.

But why do we need to pass $bits here instead of just using mem::size_of? It seems like an unnecessary risk of underfill/overfill.

@TheIronBornTheIronBorn mentioned this pull request Jun 27, 2018
Closed
@sickingsicking mentioned this pull request Jun 29, 2018
Closed
@dhardy

dhardy commented Jun 29, 2018

Copy link
Copy Markdown
Member

Since @TheIronBorn has already addressed my comments, lets merge this and make a second PR for the fixes.

@dhardy
dhardy merged commit 950c0af into rust-random:masterJun 29, 2018
@dhardydhardy mentioned this pull request Jul 2, 2018
Closed
@dhardydhardy mentioned this pull request Jul 13, 2018
Closed
28 tasks
@pitdicker
pitdicker deleted the simd_support_basic branch July 20, 2018 16:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes
+1 more reviewer
@TheIronBornTheIronBornTheIronBorn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker@TheIronBorn@dhardy
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Add basic SIMD support by pitdicker · Pull Request #523 · rust-random/rand · GitHub
Skip to content
Add basic SIMD support - #523
Merged
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Jun 29, 2018
Merged
Add basic SIMD support#523
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Conversation
@pitdicker
@pitdickerpitdicker
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
This is a rebased version of the branch from #377 (comment).
@TheIronBorn Do you want to review?
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated

 // those are usually more random.
 let float_size = mem::size_of::<$f_scalar>() * 8;
 let precision = $fraction_bits + 1;
 let scale = $ty::splat(1.0 / ((1 as $u_scalar << precision) as $f_scalar));
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: most arithmetic and bitwise operations allow you to just use scalar values:
let scale = $ty::splat(1.0 / ((1as $u_scalar << precision)as $f_scalar));
scale * value
// is the same as
let scale = 1.0 / ((1as $u_scalar << precision)as $f_scalar);
scale * value
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated


 let value: $uty = rng.gen();
 let fraction = value >> (float_size - $fraction_bits);
 fraction.into_float_with_exponent(0) - $ty::splat(1.0 - EPSILON / 2.0)
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This would be the same:
fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
spelling https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L502
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Bug: https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L600
assert!(low < high,"Uniform::new called with `low >= high`");
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
This will be fine on most inputs, but for inputs like new(f32x2::new(0.0, 100.0), f32x2::new(100.0, 0.0)) this assert will pass. We should probably have tests for these sort of cases.
The assertions need to be of the form low.lt(high).all()
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
O wow, that is something easy to get wrong! Thank you for reviewing.
I have added a math_helpers module, with:
  • The WideningMultiply implementation from Uniform;
  • A trait for casting integers to floats. Normal floats need to use as, From is not implemented for the integer sizes we use. Vectors don't have as, and need From...
  • A trait to have natural comparison for both floats, and vectors. I only implemented the few things we need.
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
The CI fails because of rust-lang/rust#51699.
dhardy
dhardy
reviewed
Jun 25, 2018
View reviewed changes
@dhardydhardy
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just some nits; meant to post this a couple of days ago
Comment threadsrc/distributions/mod.rs
Outdated

mod integer;
#[cfg(feature="std")]
mod log_gamma;
mod math_helpers;
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any more mathematical than other parts of the library? You could just call it num_utils (i.e. Number Theory, though arguably that still covers most of the library) or arithmetic.
Comment threadsrc/distributions/math_helpers.rs
Outdated


/// `PartialOrd` for vectors compares lexicographically. We want natural order.
/// Only the comparison functions we need are implemented.
pub trait NaturalCompare {
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is natural depends on the context which is how PartialOrd confused you. How about SimultaneousOrd?
@dhardy
dhardy
commented
Jun 26, 2018

Copy link


Copy Markdown

Member
@TheIronBorn do you think this is ready?
I'd like to go over it in more detail myself still, but I think it's pretty neat that we can do this (it's well beyond what most random libs can offer)!
@TheIronBorn
TheIronBorn
commented
Jun 26, 2018

Copy link


Copy Markdown

Contributor
Looks good! I'll make an SIMD swap_bytes pull request once this merges
dhardy
dhardy
approved these changes
Jun 27, 2018
View reviewed changes
@dhardydhardy
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other than those two things looks good.
BTW @TheIronBorn you can make a commit on top of @pitdicker's branch and push to your own branch if you have a fix, but best coordinate between yourselves.
Comment threadsrc/distributions/integer.rs
 unsafe {
 let ptr = &mut vec;
 let b_ptr = &mut *(ptr as *mut $ty as *mut [u8; $bits/8]);
 rng.fill_bytes(b_ptr);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look portable to me. Elsewhere we've made an effort to keep things portable; I don't think this needs to be an exception?
Unfortunately it doesn't look like the SIMD types support to_le. @TheIronBorn is this what you mean about using swap_bytes?
@TheIronBornTheIronBornJun 27, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that’s exactly where we’d use it
Comment threadsrc/distributions/integer.rs
macro_rules! simd_impl {
 ($bits:expr,) => {};
 ($bits:expr, $ty:ty, $($ty_more:ty,)*) => {
 simd_impl!($bits, $($ty_more,)*);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat usage of recursive macros.
But why do we need to pass $bits here instead of just using mem::size_of? It seems like an unnecessary risk of underfill/overfill.
@TheIronBornTheIronBorn
mentioned this pull request
Jun 27, 2018
 Closed

@sickingsicking
mentioned this pull request
Jun 29, 2018
 Closed

@dhardy
dhardy
commented
Jun 29, 2018

Copy link


Copy Markdown

Member
Since @TheIronBorn has already addressed my comments, lets merge this and make a second PR for the fixes.
@dhardy
dhardy
merged commit 950c0af
into
rust-random:masterJun 29, 2018
@dhardydhardy
mentioned this pull request
Jul 2, 2018

Closed

@dhardydhardy
mentioned this pull request
Jul 13, 2018

Closed

28 tasks
@pitdicker
pitdicker
deleted the

simd_support_basic

branch
July 20, 2018 16:30
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes

+1 more reviewer

@TheIronBornTheIronBornTheIronBorn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker@TheIronBorn@dhardy

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Add basic SIMD support by pitdicker · Pull Request #523 · rust-random/rand · GitHub
Skip to content
Add basic SIMD support - #523
Merged
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Jun 29, 2018
Merged
Add basic SIMD support#523
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Conversation
@pitdicker
@pitdickerpitdicker
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
This is a rebased version of the branch from #377 (comment).
@TheIronBorn Do you want to review?
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated

 // those are usually more random.
 let float_size = mem::size_of::<$f_scalar>() * 8;
 let precision = $fraction_bits + 1;
 let scale = $ty::splat(1.0 / ((1 as $u_scalar << precision) as $f_scalar));
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: most arithmetic and bitwise operations allow you to just use scalar values:
let scale = $ty::splat(1.0 / ((1as $u_scalar << precision)as $f_scalar));
scale * value
// is the same as
let scale = 1.0 / ((1as $u_scalar << precision)as $f_scalar);
scale * value
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated


 let value: $uty = rng.gen();
 let fraction = value >> (float_size - $fraction_bits);
 fraction.into_float_with_exponent(0) - $ty::splat(1.0 - EPSILON / 2.0)
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This would be the same:
fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
spelling https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L502
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Bug: https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L600
assert!(low < high,"Uniform::new called with `low >= high`");
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
This will be fine on most inputs, but for inputs like new(f32x2::new(0.0, 100.0), f32x2::new(100.0, 0.0)) this assert will pass. We should probably have tests for these sort of cases.
The assertions need to be of the form low.lt(high).all()
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
O wow, that is something easy to get wrong! Thank you for reviewing.
I have added a math_helpers module, with:
  • The WideningMultiply implementation from Uniform;
  • A trait for casting integers to floats. Normal floats need to use as, From is not implemented for the integer sizes we use. Vectors don't have as, and need From...
  • A trait to have natural comparison for both floats, and vectors. I only implemented the few things we need.
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
The CI fails because of rust-lang/rust#51699.
dhardy
dhardy
reviewed
Jun 25, 2018
View reviewed changes
@dhardydhardy
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just some nits; meant to post this a couple of days ago
Comment threadsrc/distributions/mod.rs
Outdated

mod integer;
#[cfg(feature="std")]
mod log_gamma;
mod math_helpers;
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any more mathematical than other parts of the library? You could just call it num_utils (i.e. Number Theory, though arguably that still covers most of the library) or arithmetic.
Comment threadsrc/distributions/math_helpers.rs
Outdated


/// `PartialOrd` for vectors compares lexicographically. We want natural order.
/// Only the comparison functions we need are implemented.
pub trait NaturalCompare {
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is natural depends on the context which is how PartialOrd confused you. How about SimultaneousOrd?
@dhardy
dhardy
commented
Jun 26, 2018

Copy link


Copy Markdown

Member
@TheIronBorn do you think this is ready?
I'd like to go over it in more detail myself still, but I think it's pretty neat that we can do this (it's well beyond what most random libs can offer)!
@TheIronBorn
TheIronBorn
commented
Jun 26, 2018

Copy link


Copy Markdown

Contributor
Looks good! I'll make an SIMD swap_bytes pull request once this merges
dhardy
dhardy
approved these changes
Jun 27, 2018
View reviewed changes
@dhardydhardy
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other than those two things looks good.
BTW @TheIronBorn you can make a commit on top of @pitdicker's branch and push to your own branch if you have a fix, but best coordinate between yourselves.
Comment threadsrc/distributions/integer.rs
 unsafe {
 let ptr = &mut vec;
 let b_ptr = &mut *(ptr as *mut $ty as *mut [u8; $bits/8]);
 rng.fill_bytes(b_ptr);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look portable to me. Elsewhere we've made an effort to keep things portable; I don't think this needs to be an exception?
Unfortunately it doesn't look like the SIMD types support to_le. @TheIronBorn is this what you mean about using swap_bytes?
@TheIronBornTheIronBornJun 27, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that’s exactly where we’d use it
Comment threadsrc/distributions/integer.rs
macro_rules! simd_impl {
 ($bits:expr,) => {};
 ($bits:expr, $ty:ty, $($ty_more:ty,)*) => {
 simd_impl!($bits, $($ty_more,)*);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat usage of recursive macros.
But why do we need to pass $bits here instead of just using mem::size_of? It seems like an unnecessary risk of underfill/overfill.
@TheIronBornTheIronBorn
mentioned this pull request
Jun 27, 2018
 Closed

@sickingsicking
mentioned this pull request
Jun 29, 2018
 Closed

@dhardy
dhardy
commented
Jun 29, 2018

Copy link


Copy Markdown

Member
Since @TheIronBorn has already addressed my comments, lets merge this and make a second PR for the fixes.
@dhardy
dhardy
merged commit 950c0af
into
rust-random:masterJun 29, 2018
@dhardydhardy
mentioned this pull request
Jul 2, 2018

Closed

@dhardydhardy
mentioned this pull request
Jul 13, 2018

Closed

28 tasks
@pitdicker
pitdicker
deleted the

simd_support_basic

branch
July 20, 2018 16:30
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes

+1 more reviewer

@TheIronBornTheIronBornTheIronBorn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker@TheIronBorn@dhardy

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Add basic SIMD support by pitdicker · Pull Request #523 · rust-random/rand · GitHub
Skip to content
Add basic SIMD support - #523
Merged
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Jun 29, 2018
Merged
Add basic SIMD support#523
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Conversation
@pitdicker
@pitdickerpitdicker
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
This is a rebased version of the branch from #377 (comment).
@TheIronBorn Do you want to review?
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated

 // those are usually more random.
 let float_size = mem::size_of::<$f_scalar>() * 8;
 let precision = $fraction_bits + 1;
 let scale = $ty::splat(1.0 / ((1 as $u_scalar << precision) as $f_scalar));
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: most arithmetic and bitwise operations allow you to just use scalar values:
let scale = $ty::splat(1.0 / ((1as $u_scalar << precision)as $f_scalar));
scale * value
// is the same as
let scale = 1.0 / ((1as $u_scalar << precision)as $f_scalar);
scale * value
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated


 let value: $uty = rng.gen();
 let fraction = value >> (float_size - $fraction_bits);
 fraction.into_float_with_exponent(0) - $ty::splat(1.0 - EPSILON / 2.0)
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This would be the same:
fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
spelling https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L502
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Bug: https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L600
assert!(low < high,"Uniform::new called with `low >= high`");
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
This will be fine on most inputs, but for inputs like new(f32x2::new(0.0, 100.0), f32x2::new(100.0, 0.0)) this assert will pass. We should probably have tests for these sort of cases.
The assertions need to be of the form low.lt(high).all()
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
O wow, that is something easy to get wrong! Thank you for reviewing.
I have added a math_helpers module, with:
  • The WideningMultiply implementation from Uniform;
  • A trait for casting integers to floats. Normal floats need to use as, From is not implemented for the integer sizes we use. Vectors don't have as, and need From...
  • A trait to have natural comparison for both floats, and vectors. I only implemented the few things we need.
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
The CI fails because of rust-lang/rust#51699.
dhardy
dhardy
reviewed
Jun 25, 2018
View reviewed changes
@dhardydhardy
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just some nits; meant to post this a couple of days ago
Comment threadsrc/distributions/mod.rs
Outdated

mod integer;
#[cfg(feature="std")]
mod log_gamma;
mod math_helpers;
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any more mathematical than other parts of the library? You could just call it num_utils (i.e. Number Theory, though arguably that still covers most of the library) or arithmetic.
Comment threadsrc/distributions/math_helpers.rs
Outdated


/// `PartialOrd` for vectors compares lexicographically. We want natural order.
/// Only the comparison functions we need are implemented.
pub trait NaturalCompare {
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is natural depends on the context which is how PartialOrd confused you. How about SimultaneousOrd?
@dhardy
dhardy
commented
Jun 26, 2018

Copy link


Copy Markdown

Member
@TheIronBorn do you think this is ready?
I'd like to go over it in more detail myself still, but I think it's pretty neat that we can do this (it's well beyond what most random libs can offer)!
@TheIronBorn
TheIronBorn
commented
Jun 26, 2018

Copy link


Copy Markdown

Contributor
Looks good! I'll make an SIMD swap_bytes pull request once this merges
dhardy
dhardy
approved these changes
Jun 27, 2018
View reviewed changes
@dhardydhardy
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other than those two things looks good.
BTW @TheIronBorn you can make a commit on top of @pitdicker's branch and push to your own branch if you have a fix, but best coordinate between yourselves.
Comment threadsrc/distributions/integer.rs
 unsafe {
 let ptr = &mut vec;
 let b_ptr = &mut *(ptr as *mut $ty as *mut [u8; $bits/8]);
 rng.fill_bytes(b_ptr);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look portable to me. Elsewhere we've made an effort to keep things portable; I don't think this needs to be an exception?
Unfortunately it doesn't look like the SIMD types support to_le. @TheIronBorn is this what you mean about using swap_bytes?
@TheIronBornTheIronBornJun 27, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that’s exactly where we’d use it
Comment threadsrc/distributions/integer.rs
macro_rules! simd_impl {
 ($bits:expr,) => {};
 ($bits:expr, $ty:ty, $($ty_more:ty,)*) => {
 simd_impl!($bits, $($ty_more,)*);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat usage of recursive macros.
But why do we need to pass $bits here instead of just using mem::size_of? It seems like an unnecessary risk of underfill/overfill.
@TheIronBornTheIronBorn
mentioned this pull request
Jun 27, 2018
 Closed

@sickingsicking
mentioned this pull request
Jun 29, 2018
 Closed

@dhardy
dhardy
commented
Jun 29, 2018

Copy link


Copy Markdown

Member
Since @TheIronBorn has already addressed my comments, lets merge this and make a second PR for the fixes.
@dhardy
dhardy
merged commit 950c0af
into
rust-random:masterJun 29, 2018
@dhardydhardy
mentioned this pull request
Jul 2, 2018

Closed

@dhardydhardy
mentioned this pull request
Jul 13, 2018

Closed

28 tasks
@pitdicker
pitdicker
deleted the

simd_support_basic

branch
July 20, 2018 16:30
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes

+1 more reviewer

@TheIronBornTheIronBornTheIronBorn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker@TheIronBorn@dhardy

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Add basic SIMD support by pitdicker · Pull Request #523 · rust-random/rand · GitHub
Skip to content
Add basic SIMD support - #523
Merged
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Jun 29, 2018
Merged
Add basic SIMD support#523
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Conversation
@pitdicker
@pitdickerpitdicker
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
This is a rebased version of the branch from #377 (comment).
@TheIronBorn Do you want to review?
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated

 // those are usually more random.
 let float_size = mem::size_of::<$f_scalar>() * 8;
 let precision = $fraction_bits + 1;
 let scale = $ty::splat(1.0 / ((1 as $u_scalar << precision) as $f_scalar));
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: most arithmetic and bitwise operations allow you to just use scalar values:
let scale = $ty::splat(1.0 / ((1as $u_scalar << precision)as $f_scalar));
scale * value
// is the same as
let scale = 1.0 / ((1as $u_scalar << precision)as $f_scalar);
scale * value
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated


 let value: $uty = rng.gen();
 let fraction = value >> (float_size - $fraction_bits);
 fraction.into_float_with_exponent(0) - $ty::splat(1.0 - EPSILON / 2.0)
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This would be the same:
fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
spelling https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L502
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Bug: https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L600
assert!(low < high,"Uniform::new called with `low >= high`");
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
This will be fine on most inputs, but for inputs like new(f32x2::new(0.0, 100.0), f32x2::new(100.0, 0.0)) this assert will pass. We should probably have tests for these sort of cases.
The assertions need to be of the form low.lt(high).all()
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
O wow, that is something easy to get wrong! Thank you for reviewing.
I have added a math_helpers module, with:
  • The WideningMultiply implementation from Uniform;
  • A trait for casting integers to floats. Normal floats need to use as, From is not implemented for the integer sizes we use. Vectors don't have as, and need From...
  • A trait to have natural comparison for both floats, and vectors. I only implemented the few things we need.
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
The CI fails because of rust-lang/rust#51699.
dhardy
dhardy
reviewed
Jun 25, 2018
View reviewed changes
@dhardydhardy
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just some nits; meant to post this a couple of days ago
Comment threadsrc/distributions/mod.rs
Outdated

mod integer;
#[cfg(feature="std")]
mod log_gamma;
mod math_helpers;
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any more mathematical than other parts of the library? You could just call it num_utils (i.e. Number Theory, though arguably that still covers most of the library) or arithmetic.
Comment threadsrc/distributions/math_helpers.rs
Outdated


/// `PartialOrd` for vectors compares lexicographically. We want natural order.
/// Only the comparison functions we need are implemented.
pub trait NaturalCompare {
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is natural depends on the context which is how PartialOrd confused you. How about SimultaneousOrd?
@dhardy
dhardy
commented
Jun 26, 2018

Copy link


Copy Markdown

Member
@TheIronBorn do you think this is ready?
I'd like to go over it in more detail myself still, but I think it's pretty neat that we can do this (it's well beyond what most random libs can offer)!
@TheIronBorn
TheIronBorn
commented
Jun 26, 2018

Copy link


Copy Markdown

Contributor
Looks good! I'll make an SIMD swap_bytes pull request once this merges
dhardy
dhardy
approved these changes
Jun 27, 2018
View reviewed changes
@dhardydhardy
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other than those two things looks good.
BTW @TheIronBorn you can make a commit on top of @pitdicker's branch and push to your own branch if you have a fix, but best coordinate between yourselves.
Comment threadsrc/distributions/integer.rs
 unsafe {
 let ptr = &mut vec;
 let b_ptr = &mut *(ptr as *mut $ty as *mut [u8; $bits/8]);
 rng.fill_bytes(b_ptr);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look portable to me. Elsewhere we've made an effort to keep things portable; I don't think this needs to be an exception?
Unfortunately it doesn't look like the SIMD types support to_le. @TheIronBorn is this what you mean about using swap_bytes?
@TheIronBornTheIronBornJun 27, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that’s exactly where we’d use it
Comment threadsrc/distributions/integer.rs
macro_rules! simd_impl {
 ($bits:expr,) => {};
 ($bits:expr, $ty:ty, $($ty_more:ty,)*) => {
 simd_impl!($bits, $($ty_more,)*);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat usage of recursive macros.
But why do we need to pass $bits here instead of just using mem::size_of? It seems like an unnecessary risk of underfill/overfill.
@TheIronBornTheIronBorn
mentioned this pull request
Jun 27, 2018
 Closed

@sickingsicking
mentioned this pull request
Jun 29, 2018
 Closed

@dhardy
dhardy
commented
Jun 29, 2018

Copy link


Copy Markdown

Member
Since @TheIronBorn has already addressed my comments, lets merge this and make a second PR for the fixes.
@dhardy
dhardy
merged commit 950c0af
into
rust-random:masterJun 29, 2018
@dhardydhardy
mentioned this pull request
Jul 2, 2018

Closed

@dhardydhardy
mentioned this pull request
Jul 13, 2018

Closed

28 tasks
@pitdicker
pitdicker
deleted the

simd_support_basic

branch
July 20, 2018 16:30
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes

+1 more reviewer

@TheIronBornTheIronBornTheIronBorn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker@TheIronBorn@dhardy

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Add basic SIMD support by pitdicker · Pull Request #523 · rust-random/rand · GitHub
Skip to content
Add basic SIMD support - #523
Merged
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Jun 29, 2018
Merged
Add basic SIMD support#523
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Conversation
@pitdicker
@pitdickerpitdicker
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
This is a rebased version of the branch from #377 (comment).
@TheIronBorn Do you want to review?
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated

 // those are usually more random.
 let float_size = mem::size_of::<$f_scalar>() * 8;
 let precision = $fraction_bits + 1;
 let scale = $ty::splat(1.0 / ((1 as $u_scalar << precision) as $f_scalar));
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: most arithmetic and bitwise operations allow you to just use scalar values:
let scale = $ty::splat(1.0 / ((1as $u_scalar << precision)as $f_scalar));
scale * value
// is the same as
let scale = 1.0 / ((1as $u_scalar << precision)as $f_scalar);
scale * value
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated


 let value: $uty = rng.gen();
 let fraction = value >> (float_size - $fraction_bits);
 fraction.into_float_with_exponent(0) - $ty::splat(1.0 - EPSILON / 2.0)
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This would be the same:
fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
spelling https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L502
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Bug: https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L600
assert!(low < high,"Uniform::new called with `low >= high`");
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
This will be fine on most inputs, but for inputs like new(f32x2::new(0.0, 100.0), f32x2::new(100.0, 0.0)) this assert will pass. We should probably have tests for these sort of cases.
The assertions need to be of the form low.lt(high).all()
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
O wow, that is something easy to get wrong! Thank you for reviewing.
I have added a math_helpers module, with:
  • The WideningMultiply implementation from Uniform;
  • A trait for casting integers to floats. Normal floats need to use as, From is not implemented for the integer sizes we use. Vectors don't have as, and need From...
  • A trait to have natural comparison for both floats, and vectors. I only implemented the few things we need.
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
The CI fails because of rust-lang/rust#51699.
dhardy
dhardy
reviewed
Jun 25, 2018
View reviewed changes
@dhardydhardy
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just some nits; meant to post this a couple of days ago
Comment threadsrc/distributions/mod.rs
Outdated

mod integer;
#[cfg(feature="std")]
mod log_gamma;
mod math_helpers;
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any more mathematical than other parts of the library? You could just call it num_utils (i.e. Number Theory, though arguably that still covers most of the library) or arithmetic.
Comment threadsrc/distributions/math_helpers.rs
Outdated


/// `PartialOrd` for vectors compares lexicographically. We want natural order.
/// Only the comparison functions we need are implemented.
pub trait NaturalCompare {
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is natural depends on the context which is how PartialOrd confused you. How about SimultaneousOrd?
@dhardy
dhardy
commented
Jun 26, 2018

Copy link


Copy Markdown

Member
@TheIronBorn do you think this is ready?
I'd like to go over it in more detail myself still, but I think it's pretty neat that we can do this (it's well beyond what most random libs can offer)!
@TheIronBorn
TheIronBorn
commented
Jun 26, 2018

Copy link


Copy Markdown

Contributor
Looks good! I'll make an SIMD swap_bytes pull request once this merges
dhardy
dhardy
approved these changes
Jun 27, 2018
View reviewed changes
@dhardydhardy
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other than those two things looks good.
BTW @TheIronBorn you can make a commit on top of @pitdicker's branch and push to your own branch if you have a fix, but best coordinate between yourselves.
Comment threadsrc/distributions/integer.rs
 unsafe {
 let ptr = &mut vec;
 let b_ptr = &mut *(ptr as *mut $ty as *mut [u8; $bits/8]);
 rng.fill_bytes(b_ptr);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look portable to me. Elsewhere we've made an effort to keep things portable; I don't think this needs to be an exception?
Unfortunately it doesn't look like the SIMD types support to_le. @TheIronBorn is this what you mean about using swap_bytes?
@TheIronBornTheIronBornJun 27, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that’s exactly where we’d use it
Comment threadsrc/distributions/integer.rs
macro_rules! simd_impl {
 ($bits:expr,) => {};
 ($bits:expr, $ty:ty, $($ty_more:ty,)*) => {
 simd_impl!($bits, $($ty_more,)*);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat usage of recursive macros.
But why do we need to pass $bits here instead of just using mem::size_of? It seems like an unnecessary risk of underfill/overfill.
@TheIronBornTheIronBorn
mentioned this pull request
Jun 27, 2018
 Closed

@sickingsicking
mentioned this pull request
Jun 29, 2018
 Closed

@dhardy
dhardy
commented
Jun 29, 2018

Copy link


Copy Markdown

Member
Since @TheIronBorn has already addressed my comments, lets merge this and make a second PR for the fixes.
@dhardy
dhardy
merged commit 950c0af
into
rust-random:masterJun 29, 2018
@dhardydhardy
mentioned this pull request
Jul 2, 2018

Closed

@dhardydhardy
mentioned this pull request
Jul 13, 2018

Closed

28 tasks
@pitdicker
pitdicker
deleted the

simd_support_basic

branch
July 20, 2018 16:30
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes

+1 more reviewer

@TheIronBornTheIronBornTheIronBorn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker@TheIronBorn@dhardy

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Add basic SIMD support by pitdicker · Pull Request #523 · rust-random/rand · GitHub
Skip to content
Add basic SIMD support - #523
Merged
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Jun 29, 2018
Merged
Add basic SIMD support#523
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Conversation
@pitdicker
@pitdickerpitdicker
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
This is a rebased version of the branch from #377 (comment).
@TheIronBorn Do you want to review?
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated

 // those are usually more random.
 let float_size = mem::size_of::<$f_scalar>() * 8;
 let precision = $fraction_bits + 1;
 let scale = $ty::splat(1.0 / ((1 as $u_scalar << precision) as $f_scalar));
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: most arithmetic and bitwise operations allow you to just use scalar values:
let scale = $ty::splat(1.0 / ((1as $u_scalar << precision)as $f_scalar));
scale * value
// is the same as
let scale = 1.0 / ((1as $u_scalar << precision)as $f_scalar);
scale * value
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated


 let value: $uty = rng.gen();
 let fraction = value >> (float_size - $fraction_bits);
 fraction.into_float_with_exponent(0) - $ty::splat(1.0 - EPSILON / 2.0)
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This would be the same:
fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
spelling https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L502
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Bug: https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L600
assert!(low < high,"Uniform::new called with `low >= high`");
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
This will be fine on most inputs, but for inputs like new(f32x2::new(0.0, 100.0), f32x2::new(100.0, 0.0)) this assert will pass. We should probably have tests for these sort of cases.
The assertions need to be of the form low.lt(high).all()
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
O wow, that is something easy to get wrong! Thank you for reviewing.
I have added a math_helpers module, with:
  • The WideningMultiply implementation from Uniform;
  • A trait for casting integers to floats. Normal floats need to use as, From is not implemented for the integer sizes we use. Vectors don't have as, and need From...
  • A trait to have natural comparison for both floats, and vectors. I only implemented the few things we need.
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
The CI fails because of rust-lang/rust#51699.
dhardy
dhardy
reviewed
Jun 25, 2018
View reviewed changes
@dhardydhardy
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just some nits; meant to post this a couple of days ago
Comment threadsrc/distributions/mod.rs
Outdated

mod integer;
#[cfg(feature="std")]
mod log_gamma;
mod math_helpers;
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any more mathematical than other parts of the library? You could just call it num_utils (i.e. Number Theory, though arguably that still covers most of the library) or arithmetic.
Comment threadsrc/distributions/math_helpers.rs
Outdated


/// `PartialOrd` for vectors compares lexicographically. We want natural order.
/// Only the comparison functions we need are implemented.
pub trait NaturalCompare {
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is natural depends on the context which is how PartialOrd confused you. How about SimultaneousOrd?
@dhardy
dhardy
commented
Jun 26, 2018

Copy link


Copy Markdown

Member
@TheIronBorn do you think this is ready?
I'd like to go over it in more detail myself still, but I think it's pretty neat that we can do this (it's well beyond what most random libs can offer)!
@TheIronBorn
TheIronBorn
commented
Jun 26, 2018

Copy link


Copy Markdown

Contributor
Looks good! I'll make an SIMD swap_bytes pull request once this merges
dhardy
dhardy
approved these changes
Jun 27, 2018
View reviewed changes
@dhardydhardy
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other than those two things looks good.
BTW @TheIronBorn you can make a commit on top of @pitdicker's branch and push to your own branch if you have a fix, but best coordinate between yourselves.
Comment threadsrc/distributions/integer.rs
 unsafe {
 let ptr = &mut vec;
 let b_ptr = &mut *(ptr as *mut $ty as *mut [u8; $bits/8]);
 rng.fill_bytes(b_ptr);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look portable to me. Elsewhere we've made an effort to keep things portable; I don't think this needs to be an exception?
Unfortunately it doesn't look like the SIMD types support to_le. @TheIronBorn is this what you mean about using swap_bytes?
@TheIronBornTheIronBornJun 27, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that’s exactly where we’d use it
Comment threadsrc/distributions/integer.rs
macro_rules! simd_impl {
 ($bits:expr,) => {};
 ($bits:expr, $ty:ty, $($ty_more:ty,)*) => {
 simd_impl!($bits, $($ty_more,)*);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat usage of recursive macros.
But why do we need to pass $bits here instead of just using mem::size_of? It seems like an unnecessary risk of underfill/overfill.
@TheIronBornTheIronBorn
mentioned this pull request
Jun 27, 2018
 Closed

@sickingsicking
mentioned this pull request
Jun 29, 2018
 Closed

@dhardy
dhardy
commented
Jun 29, 2018

Copy link


Copy Markdown

Member
Since @TheIronBorn has already addressed my comments, lets merge this and make a second PR for the fixes.
@dhardy
dhardy
merged commit 950c0af
into
rust-random:masterJun 29, 2018
@dhardydhardy
mentioned this pull request
Jul 2, 2018

Closed

@dhardydhardy
mentioned this pull request
Jul 13, 2018

Closed

28 tasks
@pitdicker
pitdicker
deleted the

simd_support_basic

branch
July 20, 2018 16:30
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes

+1 more reviewer

@TheIronBornTheIronBornTheIronBorn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker@TheIronBorn@dhardy

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Add basic SIMD support by pitdicker · Pull Request #523 · rust-random/rand · GitHub
Skip to content
Add basic SIMD support - #523
Merged
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Jun 29, 2018
Merged
Add basic SIMD support#523
dhardy merged 6 commits into
rust-random:masterfrom 
pitdicker:simd_support_basic
Conversation
@pitdicker
@pitdickerpitdicker
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
This is a rebased version of the branch from #377 (comment).
@TheIronBorn Do you want to review?
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated

 // those are usually more random.
 let float_size = mem::size_of::<$f_scalar>() * 8;
 let precision = $fraction_bits + 1;
 let scale = $ty::splat(1.0 / ((1 as $u_scalar << precision) as $f_scalar));
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: most arithmetic and bitwise operations allow you to just use scalar values:
let scale = $ty::splat(1.0 / ((1as $u_scalar << precision)as $f_scalar));
scale * value
// is the same as
let scale = 1.0 / ((1as $u_scalar << precision)as $f_scalar);
scale * value
TheIronBorn
TheIronBorn
reviewed
Jun 21, 2018
View reviewed changes
Comment threadsrc/distributions/float.rs
Outdated


 let value: $uty = rng.gen();
 let fraction = value >> (float_size - $fraction_bits);
 fraction.into_float_with_exponent(0) - $ty::splat(1.0 - EPSILON / 2.0)
@TheIronBornTheIronBornJun 21, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This would be the same:
fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018

Copy link


Copy Markdown

Contributor
spelling https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L502
@TheIronBorn
TheIronBorn
commented
Jun 21, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Bug: https://github.com/pitdicker/rand/blob/209836f7c31aa04be3113c7fde5b82c9be8e6e84/src/distributions/uniform.rs#L600
assert!(low < high,"Uniform::new called with `low >= high`");
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
This will be fine on most inputs, but for inputs like new(f32x2::new(0.0, 100.0), f32x2::new(100.0, 0.0)) this assert will pass. We should probably have tests for these sort of cases.
The assertions need to be of the form low.lt(high).all()
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
PartialOrdfor vectors compares two vectors lexicographically: https://github.com/gnzlbg/rfcs/blob/ppv/text/0000-ppv.md#traits-overview.
O wow, that is something easy to get wrong! Thank you for reviewing.
I have added a math_helpers module, with:
  • The WideningMultiply implementation from Uniform;
  • A trait for casting integers to floats. Normal floats need to use as, From is not implemented for the integer sizes we use. Vectors don't have as, and need From...
  • A trait to have natural comparison for both floats, and vectors. I only implemented the few things we need.
@pitdicker
pitdicker
commented
Jun 22, 2018

Copy link


Copy Markdown

ContributorAuthor
The CI fails because of rust-lang/rust#51699.
dhardy
dhardy
reviewed
Jun 25, 2018
View reviewed changes
@dhardydhardy
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just some nits; meant to post this a couple of days ago
Comment threadsrc/distributions/mod.rs
Outdated

mod integer;
#[cfg(feature="std")]
mod log_gamma;
mod math_helpers;
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any more mathematical than other parts of the library? You could just call it num_utils (i.e. Number Theory, though arguably that still covers most of the library) or arithmetic.
Comment threadsrc/distributions/math_helpers.rs
Outdated


/// `PartialOrd` for vectors compares lexicographically. We want natural order.
/// Only the comparison functions we need are implemented.
pub trait NaturalCompare {
@dhardydhardyJun 22, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is natural depends on the context which is how PartialOrd confused you. How about SimultaneousOrd?
@dhardy
dhardy
commented
Jun 26, 2018

Copy link


Copy Markdown

Member
@TheIronBorn do you think this is ready?
I'd like to go over it in more detail myself still, but I think it's pretty neat that we can do this (it's well beyond what most random libs can offer)!
@TheIronBorn
TheIronBorn
commented
Jun 26, 2018

Copy link


Copy Markdown

Contributor
Looks good! I'll make an SIMD swap_bytes pull request once this merges
dhardy
dhardy
approved these changes
Jun 27, 2018
View reviewed changes
@dhardydhardy
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other than those two things looks good.
BTW @TheIronBorn you can make a commit on top of @pitdicker's branch and push to your own branch if you have a fix, but best coordinate between yourselves.
Comment threadsrc/distributions/integer.rs
 unsafe {
 let ptr = &mut vec;
 let b_ptr = &mut *(ptr as *mut $ty as *mut [u8; $bits/8]);
 rng.fill_bytes(b_ptr);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look portable to me. Elsewhere we've made an effort to keep things portable; I don't think this needs to be an exception?
Unfortunately it doesn't look like the SIMD types support to_le. @TheIronBorn is this what you mean about using swap_bytes?
@TheIronBornTheIronBornJun 27, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that’s exactly where we’d use it
Comment threadsrc/distributions/integer.rs
macro_rules! simd_impl {
 ($bits:expr,) => {};
 ($bits:expr, $ty:ty, $($ty_more:ty,)*) => {
 simd_impl!($bits, $($ty_more,)*);
@dhardydhardyJun 27, 2018

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat usage of recursive macros.
But why do we need to pass $bits here instead of just using mem::size_of? It seems like an unnecessary risk of underfill/overfill.
@TheIronBornTheIronBorn
mentioned this pull request
Jun 27, 2018
 Closed

@sickingsicking
mentioned this pull request
Jun 29, 2018
 Closed

@dhardy
dhardy
commented
Jun 29, 2018

Copy link


Copy Markdown

Member
Since @TheIronBorn has already addressed my comments, lets merge this and make a second PR for the fixes.
@dhardy
dhardy
merged commit 950c0af
into
rust-random:masterJun 29, 2018
@dhardydhardy
mentioned this pull request
Jul 2, 2018

Closed

@dhardydhardy
mentioned this pull request
Jul 13, 2018

Closed

28 tasks
@pitdicker
pitdicker
deleted the

simd_support_basic

branch
July 20, 2018 16:30
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes

+1 more reviewer

@TheIronBornTheIronBornTheIronBorn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker@TheIronBorn@dhardy