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.

Sign up for GitHub

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

Jump to bottom

Enable GVN for AggregateKind::RawPtr #125041

Merged
merged 1 commit into from
Jun 9, 2024
Merged

Enable GVN for AggregateKind::RawPtr #125041

merged 1 commit into from
Jun 9, 2024

Conversation

scottmcm
Copy link
Member

Looks like I was worried for nothing; this seems like it's much easier than I was originally thinking it would be.
r? @cjgillot

This should be useful for x[..4]-like things, should those start inlining enough to expose the lengths.

@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. labels May 12, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 12, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@scottmcm scottmcm linked an issue May 12, 2024 that may be closed by this pull request
MIR const-prop not putting usize into AggregateKind::RawPtr? #124792
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
WaffleLapkin
WaffleLapkin reviewed May 12, 2024
View reviewed changes
compiler/rustc_mir_transform/src/gvn.rs Outdated
@@ -176,6 +176,9 @@ enum AggregateTy<'tcx> {
Array,
Tuple,
Def(DefId, ty::GenericArgsRef<'tcx>),
/// The type of the pointer can't be determined from the operands,
/// so we have to keep the full thing here.
RawPtr(Ty<'tcx>),
Copy link
Member

Choose a reason for hiding this comment

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

it's this the type of the pointer, or the pointee? (*const A vs A)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
cjgillot
cjgillot reviewed May 12, 2024
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thanks. For the desconstruction operations (cast and Len for now), do you plan to do it as a follow-up, or do you want me to do it?

compiler/rustc_mir_transform/src/gvn.rs Outdated Show resolved Hide resolved
@scottmcm scottmcm marked this pull request as draft May 29, 2024 06:57
@scottmcm scottmcm 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 May 29, 2024
@scottmcm scottmcm marked this pull request as ready for review May 30, 2024 17:01
@scottmcm
Copy link
Member Author

scottmcm commented May 30, 2024 •
edited
Loading

Ok, with UnOp::PtrMetadata landed I picked this up again and added some consumption cases too. Hopefully this is about right...

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 30, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
scottmcm
scottmcm commented Jun 1, 2024
View reviewed changes
tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_fat.PreCodegen.after.panic-abort.mir
@@ -28,11 +27,8 @@ fn demo_byte_add_fat(_1: *const [u32], _2: usize) -> *const [u32] {
_4 = Offset(_3, _2);
StorageDead(_3);
StorageLive(_5);
_5 = _4 as *const () (PtrToPtr);
Copy link
Member Author

Choose a reason for hiding this comment

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

The most recent update to this PR was me realizing that smarter GVN consumption code could obviate the need for this cast that's bugged me for a while 🙂

@rust-log-analyzer

This comment has been minimized.

Sign in to view
cjgillot
cjgillot reviewed Jun 2, 2024
View reviewed changes
compiler/rustc_mir_transform/src/gvn.rs
@@ -960,6 +1007,9 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
(UnOp::Not, Value::BinaryOp(BinOp::Ne, lhs, rhs)) => {
Value::BinaryOp(BinOp::Eq, *lhs, *rhs)
}
(UnOp::PtrMetadata, Value::Aggregate(AggregateTy::RawPtr { .. }, _, fields)) => {
return Some(fields[1]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add such a line to Rvalue::Len too?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit more complex for Len, because Len has a deref as well.

I think my preference be would to work towards removing Len, and not add a bunch more cases which we'd just delete later. Not having this for Len isn't a regression, after all, so I think it's fine without it.

compiler/rustc_mir_transform/src/gvn.rs
if was_updated && let Some(local) = self.try_as_local(fields[0], location) {
field_ops[FieldIdx::ZERO] = Operand::Copy(Place::from(local));
self.reused_locals.insert(local);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually check both constants and locals, to prefer keeping a constant when one is available. Or is it too unlikely here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I might as well for consistency. It's probably quite uncommon, though, because this is the data pointer, and those usually aren't constants. But I suppose for ZSTs it's possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

...ok, done.

@scottmcm
Copy link
Member Author

scottmcm commented Jun 6, 2024

(Rebased to make sure there's no conflicts after #125893 )

@cjgillot
Copy link
Contributor

cjgillot commented Jun 9, 2024

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2024

📌 Commit 021ccf6 has been approved by cjgillot

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 Jun 9, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 9, 2024
@jieyouxu
Rollup merge of rust-lang#125041 - scottmcm:gvn-for-from-raw-parts, r…
3a2854a 
…=cjgillot

Enable GVN for `AggregateKind::RawPtr`

Looks like I was worried for nothing; this seems like it's much easier than I was originally thinking it would be.
r? `@cjgillot`

This should be useful for `x[..4]`-like things, should those start inlining enough to expose the lengths.
This was referenced Jun 9, 2024
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
@bors
Auto merge of rust-lang#126202 - jieyouxu:rollup-2ixnizz, r=jieyouxu
503dfcf 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#125041 (Enable GVN for `AggregateKind::RawPtr`)
 - rust-lang#125253 (Add `FRAC_1_SQRT_2PI` constant to f16/f32/f64/f128)
 - rust-lang#125465 (bootstrap: vendor crates required by opt-dist to collect profiles )
 - rust-lang#125470 (Add release notes for 1.79.0)
 - rust-lang#125963 (Remove hard-coded hashes from codegen tests)
 - rust-lang#126188 (Fix documentation for `impl_common_helpers` in `run-make-support`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f000b42 into rust-lang:master Jun 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
@rust-timer
Unrolled build for rust-lang#125041
3bb68a5 
Rollup merge of rust-lang#125041 - scottmcm:gvn-for-from-raw-parts, r=cjgillot

Enable GVN for `AggregateKind::RawPtr`

Looks like I was worried for nothing; this seems like it's much easier than I was originally thinking it would be.
r? `@cjgillot`

This should be useful for `x[..4]`-like things, should those start inlining enough to expose the lengths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WaffleLapkin WaffleLapkin WaffleLapkin left review comments

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

Labels
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.
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.

MIR const-prop not putting usize into AggregateKind::RawPtr?
6 participants
@scottmcm @rustbot @rust-log-analyzer @cjgillot @bors @WaffleLapkin