Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added abs_diff for integer types. #88780

Merged
merged 3 commits into from
Oct 5, 2021
Merged

Added abs_diff for integer types. #88780

merged 3 commits into from
Oct 5, 2021

Conversation

orlp
Copy link
Contributor

@orlp orlp commented Sep 9, 2021

Closes #62111.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2021
@the8472
Copy link
Member

the8472 commented Sep 9, 2021

Does either of these compile to a psadbw (or equivalent SSE/AVX instructions for wider arrays) on x86?

fn sad_iter(a: &[u8; 8], b: &[u8; 8]) -> u32 {
  a.iter().zip(b).map(|&a, &b| a.abs_diff(b) as u32).sum()
}

fn sad_loop(a: &[u8; 8], b: &[u8; 8]) -> u32 {
  let mut sum = 0;
  for i in 0..a.len() {
      sum += a[i].abs_diff(b[i]) as u32;
  }
  sum
}

llvm does have some pattern detection to generate psadbw, so we should make sure we fall into that.

@the8472 the8472 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 9, 2021
@orlp
Copy link
Contributor Author

orlp commented Sep 9, 2021

@the8472 I can't get Rust to generate psadbw without intrinsics at all. I'm not sure if your linked thread was actually merged into LLVM or is still in the code.

@the8472
Copy link
Member

the8472 commented Sep 9, 2021

Yes, it's still there. Based on comments in tickets the pattern it expects for absolute difference should look like abs(a - b), it must be in a loop that can be vectorized and the aggregation variable must be 32bit or 64bit wide.

https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/sad.ll
https://raw.githubusercontent.com/llvm/llvm-project/main/llvm/lib/Target/X86/X86ISelLowering.cpp

@thomcc
Copy link
Member

thomcc commented Sep 9, 2021

It's a really fragile optimization. I've never seen it in practice, but: https://godbolt.org/z/qffbWvh4v

@orlp
Copy link
Contributor Author

orlp commented Sep 9, 2021

@the8472 I've tried a bunch of things, and I can't generate it.

Note that abs(a-b) is not valid at all unless you widen the type first (which I've also tried without luck).

EDIT: it appears to only work for exactly this:

pub fn abs_diff(slf: u8, other: u8)  -> u8 {
    (slf as i32).wrapping_sub(other as i32).abs() as u8
}

If you widen to anything except i32 it doesn't work at all.

@the8472
Copy link
Member

the8472 commented Sep 9, 2021

It's a really fragile optimization.

I suspect it's written to match C's integer conversion rules.

@orlp
Copy link
Contributor Author

orlp commented Sep 9, 2021

If we consider this worthwhile I could add that implementation for u8 specifically. It does mean I have to pull out from the uint_macros and create a macro myself to add the current implementation for everything except u8.

EDIT: or I could do a little hack using if mem::size_of::<Self>() == 1 and see if it affects codegen at all.

EDIT2: appears to work to generate the psad instruction.

@the8472
Copy link
Member

the8472 commented Sep 9, 2021

SAD is really important in video processing. Yes, all serious implementations will use platform-specific intrinsics but they have generic fallback paths and if those get used for any reason (wasm? riscv?) it should be in a pattern that optimizers recognize.

@orlp
Copy link
Contributor Author

orlp commented Sep 9, 2021

@the8472 Implemented psadbw support.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Oct 3, 2021

This looks good to me.

Can you open a tracking issue, and add its number in the #[unstable] attributes? Thanks!

@orlp orlp mentioned this pull request Oct 3, 2021
4 tasks
@orlp
Copy link
Contributor Author

orlp commented Oct 3, 2021

@m-ou-se Done: #89492.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 3, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2021

📌 Commit 6dd6e7c has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2021
…ingjubilee

Rollup of 15 pull requests

Successful merges:

 - rust-lang#87993 (Stabilize try_reserve)
 - rust-lang#88090 (Perform type inference in range pattern)
 - rust-lang#88780 (Added abs_diff for integer types.)
 - rust-lang#89270 (path.push() should work as expected on windows verbatim paths)
 - rust-lang#89413 (Correctly handle supertraits for min_specialization)
 - rust-lang#89456 (Update to the final LLVM 13.0.0 release)
 - rust-lang#89466 (Fix bug with query modifier parsing)
 - rust-lang#89473 (Fix extra `non_snake_case` warning for shorthand field bindings)
 - rust-lang#89474 (rustdoc: Improve doctest pass's name and module's name)
 - rust-lang#89478 (Fixed numerus of error message)
 - rust-lang#89480 (Add test for issue 89118.)
 - rust-lang#89487 (Try to recover from a `=>` -> `=` or `->` typo in a match arm)
 - rust-lang#89494 (Deny `where` clauses on `auto` traits)
 - rust-lang#89511 (:arrow_up: rust-analyzer)
 - rust-lang#89536 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021
@bors bors merged commit 234fa90 into rust-lang:master Oct 5, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a method for computing the absolute difference between unsigned integers
8 participants