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

Replacement of #114390: Add new intrinsic is_var_statically_known and optimize pow for powers of two #119911

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

NCGThompson
Copy link
Contributor

@NCGThompson NCGThompson commented Jan 12, 2024

This adds a new intrinsic is_val_statically_known that lowers to @llvm.is.constant.*. It also applies the intrinsic in the int_pow methods to recognize and optimize the idiom 2isize.pow(x). See #114390 for more discussion.

While I have extended the scope of the power of two optimization from #114390, I haven't added any new uses for the intrinsic. That can be done in later pull requests.

Note: When testing or using the library, be sure to use --stage 1 or higher. Otherwise, the intrinsic will be a noop and the doctests will be skipped. If you are trying out edits, you may be interested in --keep-stage 0.

Fixes #47234
Resolves #114390
@Centri3

@rustbot
Copy link
Collaborator

rustbot commented Jan 12, 2024

r? @Mark-Simulacrum

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 12, 2024
@NCGThompson
Copy link
Contributor Author

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned Mark-Simulacrum Jan 12, 2024
@NCGThompson
Copy link
Contributor Author

NCGThompson commented Jan 12, 2024

Assuming all the tests pass and no one has any suggestions, all that's left now is some changes to the function's documentation.

@NCGThompson NCGThompson changed the title Clone of #114390: Add new intrinsic is_var_statically_known and optimize pow for powers of two Replacement of #114390: Add new intrinsic is_var_statically_known and optimize pow for powers of two Jan 13, 2024
@NCGThompson
Copy link
Contributor Author

@oli-obk Should I be at all concerned about GCC and Cranelift?

@NCGThompson NCGThompson marked this pull request as ready for review January 13, 2024 03:48
@rustbot
Copy link
Collaborator

rustbot commented Jan 13, 2024

The Miri subtree was changed

cc @rust-lang/miri

@NCGThompson NCGThompson marked this pull request as draft January 13, 2024 17:38
@NCGThompson
Copy link
Contributor Author

Converted back to draft so I can add support in GCC and a fallback in Cranelift.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 15, 2024

The other backends can just return false (with a FIXME comment for the respective owners that they can address or remove as they see fit)

@NCGThompson
Copy link
Contributor Author

NCGThompson commented Jan 15, 2024

The other backends can just return false (with a FIXME comment for the respective owners that they can address or remove as they see fit)

Do they have to enforce correct types? For LLVM, Centri left a FIXME note essentially saying she was only allowing a finite amount of types as inputs because she had to explicitly declare each of them. I don't have that issue with Cranelift because it isn't backed by a real intrinsic anyway. Do I have to tell Cranelift to panic when it gets an argument that LLVM can't accept?

Edit: they do not

@rust-log-analyzer

This comment has been minimized.

@NCGThompson
Copy link
Contributor Author

Force undoing that commit.

@NCGThompson
Copy link
Contributor Author

Since I don't want to put tmi in the core docs and the intrinsic is a too unstable for formal documentation, I'll try to put info about it in this PR.

The rustc front-end (MIR passes) never attempts to resolve the intrinsic to true or false; it leaves it for the backend. The exception of course is in const contexts. LLVM will resolve the intrinsic to true when it reaches it while constant propagating. Later, LLVM does a special pass that resolves it to false unless it has a literal in the right hand side. After it determins the output during that pass, LLVM will propogate the output and eliminate any branches that become unreachable as a result. This pass even happens with -C opt-level 0. For all other optimization settings, that pass happens before general optimization is over.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

codegen_gcc also still needs to handle the intrinsic

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs Outdated Show resolved Hide resolved
tests/codegen/is_val_statically_known.rs Outdated Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2024
@NCGThompson
Copy link
Contributor Author

When I next get the chance, I need to edit pow et al to better take advantage of constant propagation, then edit the codegen tests to match. After that, I will mark it as ready for review.

While saturating_mul makes the Rust more concise, it has the opposite effect on the assembly. Compile-time division works better.

@NCGThompson NCGThompson marked this pull request as ready for review January 19, 2024 04:24
@NCGThompson
Copy link
Contributor Author

Now both codegen tests have -O. This should let it pass the test that just failed.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 24, 2024

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jan 24, 2024

📌 Commit 9dccd5d has been approved by oli-obk

It is now in the queue for this repository.

@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 Jan 24, 2024
@bors
Copy link
Contributor

bors commented Jan 25, 2024

⌛ Testing commit 9dccd5d with merge 039d887...

@bors
Copy link
Contributor

bors commented Jan 25, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 039d887 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 25, 2024
@bors bors merged commit 039d887 into rust-lang:master Jan 25, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 25, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (039d887): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.9% [0.9%, 0.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
9.6% [9.6%, 9.6%] 1
Improvements ✅
(primary)
-4.5% [-4.9%, -4.1%] 2
Improvements ✅
(secondary)
-4.5% [-6.6%, -3.4%] 5
All ❌✅ (primary) -4.5% [-4.9%, -4.1%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.1%] 1
All ❌✅ (primary) - - 0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.2% [-0.2%, -0.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.2% [-0.2%, -0.2%] 1

Bootstrap: 662.082s -> 661.843s (-0.04%)
Artifact size: 308.26 MiB -> 308.29 MiB (0.01%)

/// unsafe {
/// if !is_val_statically_known(0) { unreachable_unchecked(); }
/// }
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

Tests that have UB should be marked as no_run... this one is now tripping Miri, of course.

I'll file a PR.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #120366

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jan 26, 2024
…li-obk

Replacement of rust-lang#114390: Add new intrinsic `is_var_statically_known` and optimize pow for powers of two

This adds a new intrinsic `is_val_statically_known` that lowers to [``@llvm.is.constant.*`](https://llvm.org/docs/LangRef.html#llvm-is-constant-intrinsic).` It also applies the intrinsic in the int_pow methods to recognize and optimize the idiom `2isize.pow(x)`. See rust-lang#114390 for more discussion.

While I have extended the scope of the power of two optimization from rust-lang#114390, I haven't added any new uses for the intrinsic. That can be done in later pull requests.

Note: When testing or using the library, be sure to use `--stage 1` or higher. Otherwise, the intrinsic will be a noop and the doctests will be skipped. If you are trying out edits, you may be interested in [`--keep-stage 0`](https://rustc-dev-guide.rust-lang.org/building/suggested.html#faster-builds-with---keep-stage).

Fixes rust-lang#47234
Resolves rust-lang#114390
`@Centri3`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 26, 2024
…r=cuviper

mark a doctest with UB as no_run

rust-lang#119911 added a doctest with UB. That one shouldn't be run, or else Miri will complain.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 27, 2024
Rollup merge of rust-lang#120366 - RalfJung:is_val_statically_known, r=cuviper

mark a doctest with UB as no_run

rust-lang#119911 added a doctest with UB. That one shouldn't be run, or else Miri will complain.
oli-obk added a commit to oli-obk/rust that referenced this pull request Feb 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
Revert unsound libcore changes

fixes rust-lang#120537

these were introduced in rust-lang#119911
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 4, 2024
Rollup merge of rust-lang#120562 - oli-obk:revert_stuff, r=cuviper

Revert unsound libcore changes

fixes rust-lang#120537

these were introduced in rust-lang#119911
cuviper pushed a commit to cuviper/rust that referenced this pull request Feb 14, 2024
@nyurik
Copy link
Contributor

nyurik commented Feb 14, 2024

@NCGThompson I tried using this new intrinsic with core::fmt::Arguments<'_>, but it crashed rustc (big report was denied because apparently compiler crashes with intrinsics are not tracked). Is it possible to solve #121001 by detecting at compile time when the Arguments is one of these cases?

    pub const fn as_str(&self) -> Option<&'static str> {
        match (self.pieces, self.args) {
            ([], []) => Some(""),
            ([s], []) => Some(s),
            _ => None,
        }
    }

@NCGThompson
Copy link
Contributor Author

@NCGThompson I tried using this new intrinsic with core::fmt::Arguments<'_>, but it crashed rustc (big report was denied because apparently compiler crashes with intrinsics are not tracked). Is it possible to solve #121001 by detecting at compile time when the Arguments is one of these cases?

    pub const fn as_str(&self) -> Option<&'static str> {
        match (self.pieces, self.args) {
            ([], []) => Some(""),
            ([s], []) => Some(s),
            _ => None,
        }
    }

I think so. I'll have to look at #121001 more before I give you a precise answer, but there is a general trick for detecting a rare special case.

let a: bool = evaluateCondition();
if is_val_statically_known(a) && a {
    handleSpecialCase();
} else {
    handleGeneralCase();
}

Note that evaluateCondition() is ran regardless of the result of is_val_statically_known(a). If the optimizer recognizes that evaluateCondition() has no side effects, then it will likely be able to elide it after it resolves is_val_statically_known(a) to a boolean.

If I understand your code correctly, evaluateCondition() should be replaced by self.pieces.len() == 0. And of course you can replace the if-else with a match. As I mentioned I'll have to take a closer look later.

@nyurik
Copy link
Contributor

nyurik commented Feb 15, 2024

Thanks @NCGThompson ! It turns out i could reuse an existing function that returns Option<&str>, and run is_val_statically_known(val.is_some)! I was just really confused why I got a compiler crash (rather than a regular diagnostics error). Now it all makes sense :)

See https://github.com/rust-lang/rust/pull/121001/files#diff-79bafab5831564f0d33567cf953a0c6cb70c66ed5db55a70fb39f485c48858aeR445-R449

GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this pull request Feb 21, 2024
…li-obk

Replacement of rust-lang#114390: Add new intrinsic `is_var_statically_known` and optimize pow for powers of two

This adds a new intrinsic `is_val_statically_known` that lowers to [``@llvm.is.constant.*`](https://llvm.org/docs/LangRef.html#llvm-is-constant-intrinsic).` It also applies the intrinsic in the int_pow methods to recognize and optimize the idiom `2isize.pow(x)`. See rust-lang#114390 for more discussion.

While I have extended the scope of the power of two optimization from rust-lang#114390, I haven't added any new uses for the intrinsic. That can be done in later pull requests.

Note: When testing or using the library, be sure to use `--stage 1` or higher. Otherwise, the intrinsic will be a noop and the doctests will be skipped. If you are trying out edits, you may be interested in [`--keep-stage 0`](https://rustc-dev-guide.rust-lang.org/building/suggested.html#faster-builds-with---keep-stage).

Fixes rust-lang#47234
Resolves rust-lang#114390
`@Centri3`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.pow(n) does not optimize well