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

Implement rotate using funnel shift on LLVM >= 7 #55650

Merged
merged 1 commit into from Nov 10, 2018

Conversation

Projects
None yet
6 participants
@nikic
Contributor

nikic commented Nov 3, 2018

Implement the rotate_left and rotate_right operations using
llvm.fshl and llvm.fshr if they are available (LLVM >= 7).

Originally I wanted to expose the funnel_shift_left and
funnel_shift_right intrinsics and implement rotate_left and
rotate_right on top of them. However, emulation of funnel
shifts requires emitting a conditional to check for zero shift
amount, which is not necessary for rotates. I was uncomfortable
doing that here, as I don't want to rely on LLVM to optimize
away that conditional (and for variable rotates, I'm not sure it
can). We should revisit that question when we raise our minimum
version requirement to LLVM 7 and don't need emulation code
anymore.

Fixes #52457.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 3, 2018

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@nagisa

This comment has been minimized.

Contributor

nagisa commented Nov 3, 2018

FWIW the codegen on everything but x86 is abysmal with this intrinsic. It might be prudent to wait a little until intrinsic codegen is fixed in other backends.

I’ll look at how hard is it to make a patch for ARM/PPC backends in LLVM…

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned estebank Nov 3, 2018

@nikic

This comment has been minimized.

Contributor

nikic commented Nov 3, 2018

@nagisa Is that still the case? There were some changes in August to lower to ROTL/ROTR (if legal) during selectiondag construction (llvm-mirror/llvm@8fe02fa, llvm-mirror/llvm@3d464de).

@nagisa

This comment has been minimized.

Contributor

nagisa commented Nov 3, 2018

Ah, I was looking at the wrong thing. The targets I was looking at do not support byte-size rotate, and I was looking at exactly that. I think I still see some issues with the generated code, but I’ll report them against LLVM directly.

@nagisa

This comment has been minimized.

Contributor

nagisa commented Nov 3, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 3, 2018

📌 Commit 0fb2367 has been approved by nagisa

@nagisa

This comment has been minimized.

Contributor

nagisa commented Nov 3, 2018

@bors r-

r=me after the constant evaluation code is adjusted to calculate the rotation directly instead of emulating the rotation algorithm.

@nikic nikic force-pushed the nikic:funnel-shift branch from 0fb2367 to e1498e3 Nov 3, 2018

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 3, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0f67db95:start=1541279531418760389,finish=1541279587111830543,duration=55693070154
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:50:15] .................................................................................................... 100/4987
[00:50:18] .................................................................................................... 200/4987
[00:50:21] ............................................................................................ii...... 300/4987
[00:50:24] .........................................................................................iii........ 400/4987
[00:50:27] iiiiiiii.iii...........................iii...........................................i...........i.. 500/4987
[00:50:34] .................................................................................................... 700/4987
[00:50:40] ..................................................................i...........i..................... 800/4987
[00:50:43] .....................................................................................iiiii.......... 900/4987
[00:50:46] .................................................................................................... 1000/4987
---
[00:51:22] .................................................................................................... 2200/4987
[00:51:26] .................................................................................................... 2300/4987
[00:51:30] .................................................................................................... 2400/4987
[00:51:34] .................................................................................................... 2500/4987
[00:51:38] ......................................................................iiiiiiiii..................... 2600/4987
[00:51:45] .....................ii............................................................................. 2800/4987
[00:51:47] .................................................................................................... 2900/4987
[00:51:51] .................................................................................................... 3000/4987
[00:51:54] ................i................................................................................... 3100/4987
---
[00:53:00] .................................................................................................... 100/2879
[00:53:10] ...............................................................................i.................... 200/2879
[00:53:19] .................................................................................................... 300/2879
[00:53:29] .................................................................................................... 400/2879
[00:53:38] ........................F........................................................................... 500/2879
[00:54:04] .................................................................................................... 700/2879
[00:54:14] .................................................................................................... 800/2879
[00:54:24] .................................................................................................... 900/2879
[00:54:38] .................................................................................................... 1000/2879

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Implement rotate using funnel shift on LLVM >= 7
Implement the rotate_left and rotate_right operations using
llvm.fshl and llvm.fshr if they are available (LLVM >= 7).

Originally I wanted to expose the funnel_shift_left and
funnel_shift_right intrinsics and implement rotate_left and
rotate_right on top of them. However, emulation of funnel
shifts requires emitting a conditional to check for zero shift
amount, which is not necessary for rotates. I was uncomfortable
doing that here, as I don't want to rely on LLVM to optimize
away that conditional (and for variable rotates, I'm not sure it
can). We should revisit that question when we raise our minimum
version requirement to LLVM 7 and don't need emulation code
anymore.

@nikic nikic force-pushed the nikic:funnel-shift branch from e1498e3 to 4c40ff6 Nov 3, 2018

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Nov 3, 2018

@bors r=nagisa

@bors

This comment has been minimized.

Contributor

bors commented Nov 3, 2018

📌 Commit 4c40ff6 has been approved by nagisa

kennytm added a commit to kennytm/rust that referenced this pull request Nov 8, 2018

Rollup merge of rust-lang#55650 - nikic:funnel-shift, r=nagisa
Implement rotate using funnel shift on LLVM >= 7

Implement the rotate_left and rotate_right operations using
llvm.fshl and llvm.fshr if they are available (LLVM >= 7).

Originally I wanted to expose the funnel_shift_left and
funnel_shift_right intrinsics and implement rotate_left and
rotate_right on top of them. However, emulation of funnel
shifts requires emitting a conditional to check for zero shift
amount, which is not necessary for rotates. I was uncomfortable
doing that here, as I don't want to rely on LLVM to optimize
away that conditional (and for variable rotates, I'm not sure it
can). We should revisit that question when we raise our minimum
version requirement to LLVM 7 and don't need emulation code
anymore.

Fixes rust-lang#52457.

bors added a commit that referenced this pull request Nov 8, 2018

Auto merge of #55789 - kennytm:rollup, r=kennytm
Rollup of 16 pull requests

Successful merges:

 - #54993 (Support for the program data address space option of LLVM's Target Datalayout)
 - #55576 (Clarify error message for -C opt-level)
 - #55638 (Fix ICE in msg_span_from_free_region on ReEmpty)
 - #55650 (Implement rotate using funnel shift on LLVM >= 7)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55719 (Sidestep link error from rustfix'ed code by using a *defined* static.)
 - #55722 (impl_stable_hash_for: support enums and tuple structs with generic parameters)
 - #55736 (Elide anon lifetimes in conflicting impl note)
 - #55739 (Consume optimization fuel from the MIR inliner)
 - #55753 (borrow_set: remove a helper function and a clone it uses)
 - #55755 (Improve creation of 3 IndexVecs)
 - #55758 ([regression - rust2018]: unused_mut lint false positives on nightly)
 - #55760 (Remove intermediate font specs)
 - #55761 (mir: remove a hacky recursive helper function)
 - #55774 (wasm32-unknown-emscripten expects the rust_eh_personality symbol)
 - #55788 (rustc: Request ansi colors if stderr isn't a tty)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Contributor

bors commented Nov 10, 2018

⌛️ Testing commit 4c40ff6 with merge 6e9b842...

bors added a commit that referenced this pull request Nov 10, 2018

Auto merge of #55650 - nikic:funnel-shift, r=nagisa
Implement rotate using funnel shift on LLVM >= 7

Implement the rotate_left and rotate_right operations using
llvm.fshl and llvm.fshr if they are available (LLVM >= 7).

Originally I wanted to expose the funnel_shift_left and
funnel_shift_right intrinsics and implement rotate_left and
rotate_right on top of them. However, emulation of funnel
shifts requires emitting a conditional to check for zero shift
amount, which is not necessary for rotates. I was uncomfortable
doing that here, as I don't want to rely on LLVM to optimize
away that conditional (and for variable rotates, I'm not sure it
can). We should revisit that question when we raise our minimum
version requirement to LLVM 7 and don't need emulation code
anymore.

Fixes #52457.
@bors

This comment has been minimized.

Contributor

bors commented Nov 10, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 6e9b842 to master...

@bors bors merged commit 4c40ff6 into rust-lang:master Nov 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment