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

Rewrite the Visitor for non_ssa_locals #73854

Closed
wants to merge 8 commits into from
Closed

Rewrite the Visitor for non_ssa_locals #73854

wants to merge 8 commits into from

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jun 28, 2020 •
edited
Loading

Resolves #72931. Seems like we're not going to do #73732 anytime soon, so might as well clean this up while it's fresh in my head. This tries to implement what I perceived as the intent of the old code, since there were several bugs which I mention in #72931. Also, we now only look for ZSTs in the final type of the mir::Place, not in all the types of its projections besides the base local. I couldn't see a good reason for the old behavior.

@rust-highfive
Copy link
Collaborator

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2020
@ecstatic-morse
Copy link
Contributor Author

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned varkor Jun 28, 2020
@eddyb eddyb 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 Jul 15, 2020
@bors
Copy link
Contributor

bors commented Jul 21, 2020

☔ The latest upstream changes (presumably #69749) made this pull request unmergeable. Please resolve the merge conflicts.

@ecstatic-morse ecstatic-morse force-pushed the rewrite-local-analyzer branch 5 times, most recently from 8f82918 to 86bf63a Compare July 22, 2020 20:53
@ecstatic-morse
Copy link
Contributor Author

I made the requested changes @eddyb, can you take another look?

@JohnCSimon JohnCSimon 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 Aug 11, 2020
eddyb
eddyb reviewed Aug 13, 2020
View reviewed changes
src/librustc_codegen_ssa/mir/analyze.rs Outdated
Comment on lines 167 to 168
// Projections like `(*x)[12]` are allowed but not `*(x[12])`, since a `Deref` of a
// local acts like a `Copy` of that local.
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit iffy in that it doesn't mention pointers at all, which is what is being "copied", and the arbitrary example of indexing had me confused for a while, although I see what it's saying now.

I would frankly just say that a Deref is a pointer/reference Copy and be done with it.

eddyb
eddyb reviewed Aug 13, 2020
View reviewed changes
src/librustc_codegen_ssa/mir/analyze.rs Outdated
continue;
}

// Ignoring `Deref`s, the only allowed projections are reads of scalar fields.
Copy link
Member

Choose a reason for hiding this comment

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

s/allowed/SSA-capable (or similar phrasing) would help IMO.

eddyb
eddyb reviewed Aug 13, 2020
View reviewed changes
src/librustc_codegen_ssa/mir/analyze.rs
Comment on lines +33 to 35
if ty_requires_alloca(&analyzer.fx, layout) {
analyzer.not_ssa(local);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should do this first, and even keep the monomorphized TyAndLayouts around, to make it cheaper to check if "consume" field projections can be handled in an SSA way.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, huh, we have access to FunctionCx, I'm tempted to say we should create the FunctionCx with all of the local TyAndLayouts already computed (although we couldn't have placeholder LocalRefs easily I don't think?) - but maybe that's too much for this PR?

eddyb
eddyb reviewed Aug 13, 2020
View reviewed changes
src/librustc_codegen_ssa/mir/analyze.rs Outdated
Comment on lines 163 to 177
while let [ref proj_base @ .., elem] = *projection {
projection = proj_base;
self.super_projection_elem(local, proj_base, elem, context, location);

// Projections like `(*x)[12]` are allowed but not `*(x[12])`, since a `Deref` of a
// local acts like a `Copy` of that local.
if let mir::PlaceElem::Deref = elem {
has_disqualifying_projection = false;
context = PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy);
continue;
}

// Ignoring `Deref`s, the only allowed projections are reads of scalar fields.
if is_consume(context) && matches!(elem, mir::ProjectionElem::Field(..)) {
let base_ty = mir::Place::ty_from(local, proj_base, self.fx.mir, self.fx.cx.tcx());
let base_ty = self.fx.monomorphize(&base_ty);
let span = self.fx.mir.local_decls[local].source_info.span;
let layout = self.fx.cx.spanned_layout_of(base_ty.ty, span);

if !ty_requires_alloca(self.fx, layout) {
continue;
}
}

has_disqualifying_projection = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Looking more at this loop, I would split it into several parts:

  • a loop to just call super_projection_elem uniformly on all the elems, to get it out of the way (it only serves to visit the index local of an Index projection, with a `Copy context)
  • find the first Deref, if there is one, e.g. with .position(|elem| matches!(elem, mir::PlaceElem::Deref))
  • loop over the "direct projections" (i.e. before any Deref, if any) and do self.not_ssa(local); break; at the first sign of trouble

I mostly want this for clarity, but that last part has some promise for making this more efficient, so here's a few more ideas:

  • context wouldn't change across it, meaning if is_consume(context) can be placed around the whole loop
  • there could be a quick check that all the elements are fields, to avoid time even looking at the layouts
  • we probably should check all the field projections (like today / this PR) but if we go forward, from the local to the innermost field, we should almost always hit the non-SSA layout first, making the rejection cheaper
  • we could call visit_local before the loop, and skip the loop if non_ssa_locals already contains the local (see also the comment at the top of this file, which would make this more useful)
  • if we keep the monomorphized TyAndLayout of the local (which we always have to compute anyway) around, we could start from it and just go layout = layout.field(self.fx, i) at every step instead of computing the whole type and monomorphizing every time

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Aug 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I tried implementing some of these ideas in the latest commit. It's more efficient, but seems more complex to me. If you have a clear idea for how this code should look, you should implement it on top of or in place of this PR. I don't think I'm going to keep working on this.

eddyb
eddyb reviewed Aug 13, 2020
View reviewed changes
src/librustc_codegen_ssa/mir/analyze.rs Outdated
let span = self.fx.mir.local_decls[local].source_info.span;
let layout = self.fx.cx.spanned_layout_of(base_ty.ty, span);

if !ty_requires_alloca(self.fx, layout) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I feel like this doesn't capture the important aspect here: only for some layouts we can extract their fields as SSA values. So maybe instead of !ty_requires_alloca it could be layout_allows_ssa (i.e. with flipped meaning).

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Aug 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't have done the renaming in 45fd741, since being assigned exactly once before any read (SSA-like) seems to be only one of several requirements for a value to be forced to memory. I've been using the term "requires alloca" for locals that fail this check, but it feels like there should be a good term for locals that do not require allocas that I'm not seeing.

@eddyb eddyb 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 Aug 16, 2020
@ecstatic-morse ecstatic-morse force-pushed the rewrite-local-analyzer branch 2 times, most recently from c0fb44c to 99badcf Compare August 22, 2020 19:09
@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@crlf0710 crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2020
@Dylan-DPC-zz
Copy link

@ecstatic-morse if you can resolve the conflicts, we can get this merged

@Dylan-DPC-zz
Copy link

Closing this due to inactivity. Thanks

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb left review comments

Assignees

@eddyb eddyb

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor non_ssa_locals to remove LocalAnalyzer::process_place
8 participants
@ecstatic-morse @rust-highfive @bors @Dylan-DPC-zz @eddyb @crlf0710 @JohnCSimon @varkor