Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Simplify transmutes in MIR InstCombine#109612

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bors merged 1 commit intorust-lang:masterfromscottmcm:instcombine-transmutes
Mar 29, 2023

Conversation

@scottmcm
Copy link
Member

Thanks to the combination of#108246 and#108442 it could already remove identity transmutes.

With this PR, it can also simplify them toIntToInt casts,Discriminant reads, orField projections.

@rustbot
Copy link
Collaborator

r?@cjgillot

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

@rustbotrustbot added S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. T-compilerRelevant to the compiler team, which will review and decide on the PR/issue. labelsMar 25, 2023
@rustbot
Copy link
Collaborator

These commits modify theCargo.lock file. Random changes toCargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@JakobDegen
Copy link
Contributor

One actionable comment, rest lgtm

@scottmcmscottmcmforce-pushed theinstcombine-transmutes branch 2 times, most recently from169dfcc todf0e875CompareMarch 25, 2023 22:48
@bors
Copy link
Collaborator

☔ The latest upstream changes (presumably#106428) made this pull request unmergeable. Pleaseresolve the merge conflicts.

@scottmcmscottmcmforce-pushed theinstcombine-transmutes branch 4 times, most recently fromc7ec176 to36d3ea4CompareMarch 27, 2023 05:34
@rustbotrustbot added S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelsMar 27, 2023
@scottmcmscottmcmforce-pushed theinstcombine-transmutes branch from36d3ea4 to69ca902CompareMarch 28, 2023 01:58
@rustbotrustbot added S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author. labelsMar 28, 2023
@cjgillot
Copy link
Contributor

@bors r+ rollup=iffy

scottmcm reacted with heart emoji

@bors
Copy link
Collaborator

📌 Commit 69ca902a689f775ca1382738158944d7c4fa259f has been approved bycjgillot

It is now in thequeue for this repository.

@borsbors added S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelsMar 28, 2023
@bors
Copy link
Collaborator

⌛ Testing commit 69ca902a689f775ca1382738158944d7c4fa259f with merge db6c420d803168d261a0e766671cbce72d98328c...

@bors
Copy link
Collaborator

💔 Test failed -checks-actions

@borsbors removed the S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion. labelMar 28, 2023
@borsbors added the S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelMar 28, 2023
@scottmcm
Copy link
MemberAuthor

@bors retry ("Connection reset by peer (os error 104)" in test infra)

@borsbors added S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelsMar 28, 2023
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

⌛ Testing commit 69ca902a689f775ca1382738158944d7c4fa259f with merge f96b6ad273fdc9ef73e191b65d127d9a7546a0d1...

@bors
Copy link
Collaborator

💔 Test failed -checks-actions

@borsbors added S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion. labelsMar 28, 2023
@rust-log-analyzer

This comment has been minimized.

@ehuss
Copy link
Contributor

@bors r-

(was inadvertently re-queued by a homu sync)

@borsbors added S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelsMar 28, 2023
Thanks to the combination ofrust-lang#108246 andrust-lang#108442 it could already remove identity transmutes.With this PR, it can also simplify them to `IntToInt` casts, `Discriminant` reads, or `Field` projections.
@scottmcmscottmcmforce-pushed theinstcombine-transmutes branch from69ca902 tof20af8dCompareMarch 29, 2023 01:18
@scottmcm
Copy link
MemberAuthor

scottmcm commentedMar 29, 2023
edited
Loading

Rebased and made the test a// unit-test: InstCombine to hopefully not hit another difference.

@bors r=cjgillot

@bors
Copy link
Collaborator

📌 Commitf20af8d has been approved bycjgillot

It is now in thequeue for this repository.

@borsbors added S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author. labelsMar 29, 2023
@bors
Copy link
Collaborator

⌛ Testing commitf20af8d with mergeacd27bb...

@bors
Copy link
Collaborator

☀️ Test successful -checks-actions
Approved by: cjgillot
Pushingacd27bb to master...

@borsbors added the merged-by-borsThis PR was explicitly merged by bors. labelMar 29, 2023
@borsbors merged commitacd27bb intorust-lang:masterMar 29, 2023
@rustbotrustbot added this to the1.70.0 milestoneMar 29, 2023
@scottmcmscottmcm deleted the instcombine-transmutes branchMarch 29, 2023 04:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (acd27bb):comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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.

meanrangecount
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-0.1%[-0.1%, -0.1%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)-0.1%[-0.1%, -0.1%]1

Cycles

This benchmark run did not return any relevant results for this metric.

bors added a commit to rust-lang-ci/rust that referenced this pull requestMay 17, 2023
Stop turning transmutes into discriminant reads in mir-optPartially revertsrust-lang#109612, as afterrust-lang#109993 these aren't actually equivalent any more, and I'm no longer confident this was ever an improvement in the first place.Having this "simplification" meant that similar-looking code actually did somewhat different things.  For example,```rustpub unsafe fn demo1(x: std::cmp::Ordering) -> u8 {    std::mem::transmute(x)}pub unsafe fn demo2(x: std::cmp::Ordering) -> i8 {    std::mem::transmute(x)}```in nightly today is generating <https://rust.godbolt.org/z/dPK58zW18>```llvmdefine noundef i8 `@_ZN7example5demo117h341ef313673d2ee6E(i8` noundef %x) unnamed_addr #0 {  %0 = icmp uge i8 %x, -1  %1 = icmp ule i8 %x, 1  %2 = or i1 %0, %1  call void `@llvm.assume(i1` %2)  ret i8 %x}define noundef i8 `@_ZN7example5demo217h5ad29f361a3f5700E(i8` noundef %0) unnamed_addr #0 {  %x = alloca i8, align 1  store i8 %0, ptr %x, align 1  %1 = load i8, ptr %x, align 1, !range !2, !noundef !3  ret i8 %1}```Which feels too different when the original code is essentially identical.---Aside: that example is different *after* optimizations too:```llvmdefine noundef i8 `@_ZN7example5demo117h341ef313673d2ee6E(i8` noundef returned %x) unnamed_addr #0 {  %0 = add i8 %x, 1  %1 = icmp ult i8 %0, 3  tail call void `@llvm.assume(i1` %1)  ret i8 %x}define noundef i8 `@_ZN7example5demo217h5ad29f361a3f5700E(i8` noundef returned %0) unnamed_addr#1 {  ret i8 %0}```so turning the `Transmute` into a `Discriminant` was arguably just making things worse, so leaving it alone instead -- and thus having less code in rustc -- seems clearly better.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@cjgillotcjgillotcjgillot left review comments

@JakobDegenJakobDegenJakobDegen left review comments

Assignees

@cjgillotcjgillot

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.70.0

Development

Successfully merging this pull request may close these issues.

8 participants

@scottmcm@rustbot@JakobDegen@bors@cjgillot@rust-log-analyzer@ehuss@rust-timer

[8]ページ先頭

©2009-2025 Movatter.jp