Skip to content

Enable GVN for AggregateKind::RawPtr#125041

Merged
bors merged 1 commit intorust-lang:masterfrom
scottmcm:gvn-for-from-raw-parts
Jun 9, 2024
Merged

Enable GVN for AggregateKind::RawPtr#125041
bors merged 1 commit intorust-lang:masterfrom
scottmcm:gvn-for-from-raw-parts

Conversation

@scottmcm
Copy link
Copy Markdown
Member

@scottmcm scottmcm commented May 12, 2024

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
Copy Markdown
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
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
Copy Markdown
Member

@WaffleLapkin WaffleLapkin May 12, 2024

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)

@scottmcm scottmcm force-pushed the gvn-for-from-raw-parts branch from d0b9aa6 to 9d266bf Compare May 12, 2024 08:47
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@scottmcm scottmcm force-pushed the gvn-for-from-raw-parts branch from 9d266bf to b4bbd3a Compare May 12, 2024 09:12
cjgillot
cjgillot reviewed May 12, 2024
View reviewed changes
Copy link
Copy Markdown
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?

@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 force-pushed the gvn-for-from-raw-parts branch from b4bbd3a to 1881281 Compare May 30, 2024 02:19
@scottmcm scottmcm marked this pull request as ready for review May 30, 2024 17:01
@scottmcm
Copy link
Copy Markdown
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
@scottmcm scottmcm force-pushed the gvn-for-from-raw-parts branch from 1881281 to 63278d7 Compare June 1, 2024 08:05
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@scottmcm scottmcm force-pushed the gvn-for-from-raw-parts branch from 63278d7 to 94d1421 Compare June 1, 2024 08:16
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
_4 = Offset(_3, _2);
StorageDead(_3);
StorageLive(_5);
_5 = _4 as *const () (PtrToPtr);
Copy link
Copy Markdown
Member Author

@scottmcm scottmcm Jun 1, 2024

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
@scottmcm scottmcm force-pushed the gvn-for-from-raw-parts branch from 94d1421 to 5a3eb4a Compare June 1, 2024 23:57
cjgillot
cjgillot reviewed Jun 2, 2024
View reviewed changes
compiler/rustc_mir_transform/src/gvn.rs
}
(UnOp::PtrMetadata, Value::Aggregate(AggregateTy::RawPtr { .. }, _, fields)) => {
return Some(fields[1]);
}
Copy link
Copy Markdown
Contributor

@cjgillot cjgillot Jun 2, 2024

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
Copy Markdown
Member Author

@scottmcm scottmcm Jun 2, 2024

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
Copy Markdown
Contributor

@cjgillot cjgillot Jun 2, 2024

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
Copy Markdown
Member Author

@scottmcm scottmcm Jun 2, 2024

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
Copy Markdown
Member Author

@scottmcm scottmcm Jun 2, 2024

Choose a reason for hiding this comment

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

...ok, done.

@scottmcm scottmcm force-pushed the gvn-for-from-raw-parts branch from 5a3eb4a to d178208 Compare June 2, 2024 19:10
@scottmcm scottmcm force-pushed the gvn-for-from-raw-parts branch from d178208 to 021ccf6 Compare June 6, 2024 07:34
@scottmcm
Copy link
Copy Markdown
Member Author

scottmcm commented Jun 6, 2024

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

@cjgillot
Copy link
Copy Markdown
Contributor

cjgillot commented Jun 9, 2024

Thanks!
@bors r+

@bors
Copy link
Copy Markdown
Collaborator

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
@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

@cjgillot cjgillot cjgillot left review comments

@WaffleLapkin WaffleLapkin WaffleLapkin 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